Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Stop updating the app urls in the app toml when using the app management API #5246

Open
wants to merge 2 commits into
base: 01-21-patch_the_app_manifest_with_the_development_urls_add_tests
Choose a base branch
from
Open

Stop updating the app urls in the app toml when using the app management API #5246

wants to merge 2 commits into from
+11 −5

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Jan 21, 2025 •
edited
Loading

WHY are these changes introduced?

For dev sessions we don't want to update the toml, but to inject the Application URLs directly in the toml at runtime.

WHAT is this pull request doing?

Refactors the partner URL updating logic to:

  • Store new URLs in the local app for dev sessions without updating the TOML file
  • Maintain existing URL update behavior for non-dev session environments

How to test your changes?

Remember that you can find the manifest in ./shopify/bundle while running app dev

  1. Start dev with dev sessions enabled
  • Having automatically_update_urls_on_dev = true:
    • Verify that tunnel URLs are included in the manifest and not in the toml
    • Verify that modifying the URLs in the toml during dev, doesn't update them in the manifest (the manifest keeps the tunnel URL and ignores url changes from the toml)
  • Having automatically_update_urls_on_dev = false:
    • Verify that tunnel URLs are NOT included in the manifest, the manifest should include a copy of the urls value from the toml
    • Verify that modifying the URLs in the toml during dev, DO also update them in the manifest.
  1. Start dev without dev sessions
    • Verify that tunnel URLs are updated in the toml.

Measuring impact

  • 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

This was referenced Jan 21, 2025
Open
Open
@isaacroldan Graphite App
Copy link
Contributor Author

isaacroldan commented Jan 21, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@isaacroldan isaacroldan marked this pull request as ready for review January 21, 2025 13:17
@isaacroldan isaacroldan requested a review from a team as a code owner January 21, 2025 13:17
@github-actions GitHub Actions
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.

@isaacroldan isaacroldan force-pushed the 01-21-patch_the_app_manifest_with_the_development_urls_add_tests branch from b965907 to 6f9b1e8 Compare January 21, 2025 16:35
@isaacroldan isaacroldan force-pushed the 01-21-stop_updating_the_app_urls_in_the_app_toml_when_using_the_app_management_api branch 2 times, most recently from 8a036b9 to 14732be Compare January 21, 2025 16:44
craigmichaelmartin
craigmichaelmartin reviewed Jan 21, 2025
View reviewed changes
packages/app/src/cli/services/dev.ts
} else {
// When running dev app urls are pushed directly to API Client config instead of creating a new app version
// so current app version and API Client config will have diferent url values.
await updateURLs(newURLs, apiKey, developerPlatformClient, localApp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need localApp.devApplicationURLs = newURLs in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the legacy behavior that doesn't use manifests, so not needed and eventually this branch of the code should be deleted.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 21, 2025 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
75.41% (+0.03% 🔼)
 8935/11848
🟡 Branches
70.58% (-0% 🔻)
 4337/6145
🟡 Functions
75.21% (+0.05% 🔼)
 2348/3122
🟡 Lines
75.93% (+0.03% 🔼)
 8440/11115
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🔴
... / dev.ts
19.38% (-0.15% 🔻)
14.93% (-0.46% 🔻)
 24.39%
20% (-0.17% 🔻)
🔴
... / extension.ts
56.9% (-0.25% 🔻)
57.14% (-0.55% 🔻)
 50%
57.89% (-0.29% 🔻)

Test suite run success

2008 tests passing in 906 suites.

Report generated by 🧪jest coverage report action from 5dfd581

@isaacroldan isaacroldan mentioned this pull request Jan 22, 2025
Open
3 tasks
@isaacroldan isaacroldan force-pushed the 01-21-patch_the_app_manifest_with_the_development_urls_add_tests branch from 1662daa to 3eae494 Compare January 24, 2025 10:15
@isaacroldan isaacroldan force-pushed the 01-21-stop_updating_the_app_urls_in_the_app_toml_when_using_the_app_management_api branch from 14732be to 5dfd581 Compare January 24, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craigmichaelmartin craigmichaelmartin craigmichaelmartin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@isaacroldan @craigmichaelmartin