-
Notifications
You must be signed in to change notification settings - Fork 0
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
Harden versions #2136
Harden versions #2136
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📦 Next.js Bundle Analysis for @ifixit/commerce-frontendThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
This allows us to know what versions are actually being used here.
a8d5bd2
to
c7a6f1f
Compare
Hey @andyg0808 what prompted this change? Btw just for context, the purpose of version ranges for |
@dhmacs Basically, I found that we were accidentally using three different versions of Typescript. I want to use |
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.
CR ⚪
deploy_block 🔴 on checking in #dev
channel for input before proceeding?
If I'm reading the docs correctly, to upgrade a specific package one would use |
I agree using pinned versions is better (use the caret, get the stick) but to @dhmacs 's point, it sounds like these shouldn't have been installing multiple versions and it does seem better indicate the version in one place. If we're seeing multiple versions being installed, is that a bug somewhere or did we just misunderstand something? Should the packages dir be using peer deps? If the solution to this is just using syncpack then so be it, but it sounded like it was supposed to have the same result (a single shared version defined once). |
@sterlinghirsh |
@masonmcelvain I think the way you'd deal with that is update one package to the specific version you want, then use |
Also, to the specifics of the errors I saw, I think it happened because we have two different pinned versions of
|
Could you specify which packages are affected? It seems like a mistake if we've pinned different versions unintentionally. Here's what my project search shows: ![]()
The rule that we've followed is basically to use strictly pinned version under Keeping all dependencies strictly pinned is an option, but it increases maintenance overhead (having to learn syncpack, e remember to run it whenever we update dependencies). I lean towards keeping a simple approach to avoid adding complexity to the project if possible. cc @andyg0808 |
Given the pushback on this, I'm going to close it and pursue basically doing the same thing temporarily. I don't think I need to actually commit these changes to get the benefits I'm looking for. |
@danielbeardsley and I just discovered today that we're using two different versions of NextJS between I'm not moving forward with anything right now, more just want to note that the current solution doesn't seem to be entirely solving the issue. |
This configures all the versions in
react-commerce
to be specific pinned versions and not semver ranges. This makes it possible for us to work with the actual installed version numbers, instead of hoping that we're using the same version everywhere.I'm not sure that this won't cause problems long-term, but it makes
syncpack
easier to use, and I think it might lead to more stable versions installed.qa_req 0
This doesn't actually change the installed version of anything (see the
package-lock.yaml
to confirm this).