Skip to content

Specific methods for patch app configuration file scenarios #5641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shauns
Copy link
Contributor

@shauns shauns commented Apr 11, 2025

WHY are these changes introduced?

The current implementation of patchAppConfigurationFile is too broad, and we'll need something more specific if we want to switch out for a format preserving patcher.

WHAT is this pull request doing?

This PR introduces three new, more intuitive functions for modifying app configuration:

  1. setAppConfigValue - Sets a single value in the app configuration using a dotted path notation
  2. unsetAppConfigValue - Removes a value from the app configuration
  3. setManyAppConfigValues - Sets multiple values at once in the app configuration

The original patchAppConfigurationFile function is now marked as internal, and all existing usages have been updated to use the new functions. This provides a more declarative API for modifying configuration values.

How to test your changes?

  1. Run the test suite to verify all tests pass
  2. Try using the CLI to modify app configuration values (e.g., dev command that updates URLs)
  3. Verify that the TOML file is correctly updated with the expected values

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

shauns commented Apr 11, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

github-actions bot commented Apr 11, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 76.78% 9535/12419
🟡 Branches 71.83% 4681/6517
🟡 Functions 76.53% 2469/3226
🟡 Lines 77.31% 9014/11660

Test suite run success

2224 tests passing in 968 suites.

Report generated by 🧪jest coverage report action from 1407766

@shauns shauns force-pushed the shauns/04-08-specific_methods_for_patch_app_configuration_file_scenarios branch from fe1f0b4 to 767880c Compare April 22, 2025 13:42
@shauns shauns marked this pull request as ready for review April 22, 2025 13:46
@shauns shauns requested a review from a team as a code owner April 22, 2025 13:46
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@shauns shauns force-pushed the shauns/04-08-specific_methods_for_patch_app_configuration_file_scenarios branch 2 times, most recently from 953e0ef to 4209305 Compare April 23, 2025 11:37
@shauns shauns changed the base branch from main to graphite-base/5641 April 23, 2025 12:17
@shauns shauns force-pushed the shauns/04-08-specific_methods_for_patch_app_configuration_file_scenarios branch from 4209305 to 1407766 Compare April 23, 2025 12:17
@shauns shauns changed the base branch from graphite-base/5641 to shauns/04-23-remember_randomly_selected_ports_and_don_t_reuse_them April 23, 2025 12:18
Base automatically changed from shauns/04-23-remember_randomly_selected_ports_and_don_t_reuse_them to main April 23, 2025 14:55
Comment on lines +115 to +119
await setManyAppConfigValues(
app.configuration.path,
[{keyPath: 'build.dev_store_url', value: store.shopDomain}],
app.configSchema,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why many here?

Copy link
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎩 'ed and working as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants