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.

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

Use app context in functions #4616

Merged
merged 5 commits into from
Oct 10, 2024
Merged

Use app context in functions #4616

merged 5 commits into from
Oct 10, 2024

Conversation

isaacroldan
Copy link
Contributor

WHY are these changes introduced?

Start using linkedAppContext in some commands.

WHAT is this pull request doing?

  • Modifies all function commands to use the generic linkedAppContext instead of ensureConnectedAppFunctionContext
  • Create new type AppLinkedInterface for better DX
  • Update some names and types
  • Delete unnecessary tests and functions related to ensureConnectedAppFunctionContext 🚀

How to test your changes?

  • Run the function commands, here are some scenarios:
    • Logged out
    • In a new app
    • With an old toml (just a toml with client_id = 2222)

Try to run the commands in those scenarios and then repeat, the second time everything should work automatically.

Post-release steps

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
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

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

Copy link
Contributor

github-actions bot commented Oct 9, 2024

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** 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
Copy link
Contributor Author

/snapit

Copy link
Contributor

github-actions bot commented Oct 9, 2024

🫰✨ Thanks @isaacroldan! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

Copy link
Contributor

github-actions bot commented Oct 9, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.66% (-0.01% 🔻)
8533/11743
🟡 Branches
69.65% (-0% 🔻)
4191/6017
🟡 Functions 71.73% 2207/3077
🟡 Lines
73% (-0.01% 🔻)
8077/11065
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / generate-schema.ts
95% (-1.43% 🔻)
87.5% 100%
95% (-1.43% 🔻)
🟡
... / common.ts
80% (-0.95% 🔻)
62.5% 71.43%
78.95% (-1.05% 🔻)

Test suite run success

1939 tests passing in 875 suites.

Report generated by 🧪jest coverage report action from 14c8b08

@@ -26,7 +27,8 @@ interface LoadedAppContextOptions {
directory: string
forceRelink: boolean
clientId: string | undefined
configName: string | undefined
userProvidedConfigName: string | undefined
mode: AppLoaderMode
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding a default value? And could you document it in the comment above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make all options mandatory and explicit, so that each command is forced to specify how it wants to load the app, i'll add some docs to this

Copy link
Contributor

Choose a reason for hiding this comment

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

If strict is the recommended and most common way, I'd prefer to make it default to simplify, but not a strong opinion.

@isaacroldan isaacroldan added this pull request to the merge queue Oct 10, 2024
Merged via the queue into main with commit 984e8dd Oct 10, 2024
36 checks passed
@isaacroldan isaacroldan deleted the use-app-context-in-functions branch October 10, 2024 08:04
@andrewhassan andrewhassan restored the use-app-context-in-functions branch October 11, 2024 15:14
@andrewhassan andrewhassan deleted the use-app-context-in-functions branch October 11, 2024 15:14
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