-
Notifications
You must be signed in to change notification settings - Fork 326
Upgrade body-parser in css-module #1162
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
Upgrade body-parser in css-module #1162
Conversation
|
👋 It looks like you're updating JavaScript packages that are known You can deduplicate them with the If running these commands doesn't produce a change in your yarn.lock file, A duplicate React version may cause an invalid hook call warning. React context providers usually use module-scoped globals as their |
| "dependencies": { | ||
| "@shopify/hydrogen": "latest", | ||
| "body-parser": "^1.19.1", | ||
| "compression": "^1.7.4", |
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.
There are two solutions to fix this:
- Upgrade body-parser (1.20.0) can fix it.
- Remove it.
The example doesn't use the package. It can be removed.
compression is useless. I think it can be removed too.
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'm confused. The inclusion of these packages as deps should not cause a dev server error. They are not used in the Hydrogen dev server. 🤔 Can you walk me through the reasoning here?
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.
The error is from the file: examples/css-modules/node_modules/@shopify/hydrogen/node_modules/body-parser/node_modules/http-errors/index.js:261:45)

The statuses object doesn't have message property
Take a look at the commit (6 months ago) in the http-errors.
This can explain that why it throws the error. statuses object which is from another dependency package is not the latest with http-errors package.
When upgrade body-parser into the latest(1.20.0). Then all of the dependencies are latest. So the issue disappear. @jplhomer
blittle
left a comment
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.
Thank you for finding this!
Instead of removing body-parser, I would rather that we upgrade it to the latest. You are right, the template doesn't currently need it, but it's meant as a starter. The moment someone starts to take advantage of more advanced features, like the <Form> component, it will be necessary.
|
It looks like it's no longer a problem. It would still be great to update those dependencies. @jplhomer any reason the yarn lock file was removed? |
|
We don't want |
blittle
left a comment
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.
@arlyxiao as long as body-parser is updated here, might as well also update it for the framework and in the default template.
|
I'm happy with doing that. Updated. It's not necessary to add changeset? @blittle |
blittle
left a comment
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.
Thank you @arlyxiao!
Add the changeset and you'll be in the release notes :) |
Remove yarn lock because we should not be versioning a lock file for examples.
* v1.x-2022-07: (95 commits) [ci] release v1.x-2022-07 (#1170) Try ignoring hello-world to see if it will get bumped Don't consider examples part of the workspace (#1202) Fix headers on oxygen (#1201) Add bot user agents for Seoradar and Adresults, resolves #1199 (#1200) Fix changeset updates to docker deploy documentation to resolve run issues (#1196) Upgrade body-parser (#1162) Fix path for deployments Adds ability to add more than one cookie per response (#1161) Move Demo Store to templates folder (#1132) Avoid additional div element (#1191) Whoops this should only be patch Adds preconnect <link> for CDN (#1160) Bump ejs from 3.1.6 to 3.1.7 (#1147) Fix scroll restoration when server props are changed (#1152) Typo Fixes #1165 by making a missing alt tag a console warning (#1167) Remove concurrency directive for Oxygen deployments Fix hydrogen-ui dev and build issues (#1169) ...

Description
Fixed the issue below
node version: v16.13.2
Reproduce steps:
Additional context
Before submitting the PR, please make sure you do the following:
fixes #123)yarn changeset addif this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning