-
Notifications
You must be signed in to change notification settings - Fork 143
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
Validate @shopify/shopify_function
version on build
#5153
Validate @shopify/shopify_function
version on build
#5153
Conversation
10e6428
to
e121a59
Compare
@shopify/shopify_function
version on build
c980c84
to
e57adca
Compare
This comment has been minimized.
This comment has been minimized.
7cc70c4
to
f4c7577
Compare
Coverage report
Show files with reduced coverage 🔻
Test suite run success1995 tests passing in 902 suites. Report generated by 🧪jest coverage report action from f4c7577 |
@@ -139,11 +147,32 @@ | |||
return entryPoint | |||
} | |||
|
|||
async function validateShopifyFunctionPackageVersion(fun: ExtensionInstance<FunctionConfigType>) { | |||
const packageJsonPath = await findPathUp('node_modules/@shopify/shopify_function/package.json', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tried this path with different package managers? I know that pnpm sometimes uses a different folder structure for modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not, but I was following the implementation here and assumed they validated this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, spent a bit of time messing with the various package files and node_modules folders to 🎩 this.
outputContent`Make sure you have a compatible version of the ${outputToken.yellow( | ||
'@shopify/shopify_function', | ||
)} library installed.`, | ||
[ | ||
outputContent`Add ${outputToken.green( | ||
`"@shopify/shopify_function": "~${SHOPIFY_FUNCTION_NPM_PACKAGE_MAJOR_VERSION}.0.0"`, | ||
)} to the dependencies section of the package.json file in your function's directory, if not already present.` | ||
.value, | ||
`Run your package manager's install command to update dependencies.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I be able to see these messages in the console when I encounter this error?
I do see the passed in message: Could not find the Shopify Functions JavaScript library.
I know in this PR you refactored things into an AbortError subclass so this is just a mov, but still got me thinking... where does all this outputContent end up? Even running the build command with --verbose
doesn't show it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that too when testing. It appears that the error is caught and rethrown here. I didn't want to tackle that as part of this PR though, since it's more related to the general error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to fix that in a follow-up PR? there's no reason to have a custom Error if doesn't work as expected :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look. I'm not actually sure why we rethrow the error like this 😅 It looks like you added it about a month ago. Do you have context on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I added that try/catch because we were leaking some errors, and we want to properly Abort if there is an issue in any step of the build process.
But the rethrow keeps the message, why isn't it preserved in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense to me and I was able to verify the expected error message is displayed for function build and app dev for a function with shopify-functions version 0.1.0.
WHY are these changes introduced?
Closes https://github.com/Shopify/shopify-functions/issues/519
The
@shopify/shopify_function
NPM package for JavaScript Functions is implicitly tied to the version of Javy used by the CLI. However, we don't validate that the NPM package is compatible today, which means it's possible to use an older version of the NPM package that isn't compatible with a newer version of Javy.WHAT is this pull request doing?
To guarantee compatibility, this PR validates that the installed
@shopify/shopify_function
package's major version is what the CLI expects.This PR also ensures that we're using the latest patch version in our recommendations and when generating a function.
How to test your changes?
app function build
for a JS function usingshopify_function
version 1.x.y.app function build
for a JS function usingshopify_function
version 0.x.y.app dev
for a JS function (both scenarios above)app function build
for a Rust function (should not be affected by these changes)Measuring impact
How do we know this change was effective? Please choose one:
Checklist