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

meta: pinned and updated dependencies + class-fixed snapshots #5488

Merged
merged 24 commits into from
Jul 12, 2023
Merged

meta: pinned and updated dependencies + class-fixed snapshots #5488

merged 24 commits into from
Jul 12, 2023

Conversation

ovflowd
Copy link
Member

@ovflowd ovflowd commented Jul 9, 2023

This PR simply updates a few dependencies.

@ovflowd ovflowd requested a review from a team as a code owner July 9, 2023 13:49
@vercel
Copy link

vercel bot commented Jul 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2023 7:13pm
nodejs-org-stories ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2023 7:13pm

@vercel vercel bot temporarily deployed to Preview – nodejs-org July 9, 2023 13:55 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 9, 2023 13:58 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 9, 2023 14:10 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 9, 2023 14:10 Inactive
@bmuenzenmeyer
Copy link
Collaborator

Pardon my confusion. This did not update the package.json - so we are only updating resolved careted dependencies? I think I keep tripping on the strategy here, and think we should be far more explicit with the dependencies wwe manage.

Also, it's a shame the diff is almost entirely scoped CSS class name changes. I wonder if we can solve for that in a way that doesn't introduce a new authoring approach (holy war)

@ovflowd
Copy link
Member Author

ovflowd commented Jul 9, 2023

Also, it's a shame the diff is almost entirely scoped CSS class name changes. I wonder if we can solve for that in a way that doesn't introduce a new authoring approach (holy war)

If I recall correctly, we can configure Jest/Storybook to remove these generated class names. It's quite confusing that some random minor version change caused this,

@ovflowd
Copy link
Member Author

ovflowd commented Jul 9, 2023

Pardon my confusion. This did not update the package.json - so we are only updating resolved careted dependencies? I think I keep tripping on the strategy here, and think we should be far more explicit with the dependencies wwe manage.

Yes, I've ran npm outdated (to check what is outdated) then npm update (to actually update the packages).

I think this is super verbose-ish and manual. So maybe we could consider re-enabling Dependabot or use Renovate Bot. (I personally prefer Renovat over Dependabot)

@ovflowd
Copy link
Member Author

ovflowd commented Jul 9, 2023

@bmuenzenmeyer I've pinned the dependencies, as per #5491

@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 9, 2023 22:03 Inactive
Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

I'd love to learn more about the rationale between some of these decisions. We should probably document it.

Why:

  • dependencies vs devDependencies

When to:

  • pin (explicit semver)
  • caret: MINOR and PATCH
  • tilde: PATCH

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jul 9, 2023

Pinning dependencies is harmful (and due to transitive deps, doesn't actually offer much value); lockfiles are the most robust solution available.
#5491 (comment)

@bmuenzenmeyer
Copy link
Collaborator

Maybe we (or I) are splitting hairs, and if the team resolves to keep the current strategy, so be it. But updates like the first commits here that only had lockfile updates to me are worse. No human is reviewing that. There is no intentionality to it. We are trusting semver and the community to follow through (usually a good thing). We are trusting our CI (which is always a good thing).

Issue logs are littered with counter-points. Need an example? Jest just broke CI systems across the world, including at my office, held up a release of our own, and had multiple engineers wasting a day to track down a transient dependency update.

This is all about introducing more determinism into the stack, even if only a measure more. Does it solve everything? No. Should we continue to use lockfiles? Of course.

@ovflowd
Copy link
Member Author

ovflowd commented Jul 9, 2023

I'm leaning toward agreeing with @bmuenzenmeyer here, as we suffered from these same issues in this repository. Right now, even on this PR.

Tests started to fail because one of these dependencies that got updated (minor or patch) broke something. Actually, I need to give an 👀 to see if we can exclude classNames or CSS class names from the snapshots. (That itself is a bad practice)

+ Other issues in the past were also caused by transient dependencies.

@ovflowd
Copy link
Member Author

ovflowd commented Jul 11, 2023

I'd love to learn more about the rationale between some of these decisions. We should probably document it.

Yes, we probably should do that.

@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 11, 2023 11:23 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 11, 2023 11:25 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jul 11, 2023

📦 Next.js Bundle Analysis for nodejs.org

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 11, 2023 11:25 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 11, 2023 11:31 Inactive
@ljharb
Copy link
Member

ljharb commented Jul 11, 2023

If that's the case, then certainly you should pin it in package.json with =, explicitly - but only on deps where the updates are problematic.

@vercel vercel bot temporarily deployed to Preview – nodejs-org July 11, 2023 13:11 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 11, 2023 13:11 Inactive
@ovflowd
Copy link
Member Author

ovflowd commented Jul 11, 2023

If that's the case, then certainly you should pin it in package.json with =, explicitly - but only on deps where the updates are problematic.

Agreed, and I've done that. But some dependencies just really don't need to have updates. The more dependencies we have and the less "strict" we become with defining those dependency ranges, the more work we have on reviewing these updates.

For example, we don't necessarily want the latest and greatest features from TypeScript. If we find anything cooler that is not covered by 5.1.X, then we can manually bump that. Or, for example, why would we need to have a ~ or a ^ operator for glob or sass or husky? It works perfectly with the versions they are, and we don't need them to be updated.

The same applies to eslint and jest. Right now the version we are contain everything we need.

@richardlau
Copy link
Member

For example, we don't necessarily want the latest and greatest features from TypeScript.

Typescript is an outlier that does not follow semver -- that should definitely be pinned.

@ljharb
Copy link
Member

ljharb commented Jul 11, 2023

Indeed, TS should use ~ for that reason.

@ljharb
Copy link
Member

ljharb commented Jul 11, 2023

That said, why wouldn't you always want the latest features?

@ovflowd
Copy link
Member Author

ovflowd commented Jul 11, 2023

That said, why wouldn't you always want the latest features?

I think I've explained in my comment before and on the Markdown doc that I've introduced 🤔

Some packages are just fine as they are, and we don't need anything new. Many packages are notorious for breaking stuff on minor changes. Jest, Storybook, etc. Suppose you give an 👀 to the package.json of this PR, I would say that all the packages that make sense always to have the latest and greatest are there, and the others I've applied pretty "fair" policies (IMHO)

@ovflowd ovflowd changed the title chore: updated dependencies meta: pinned and updated dependencies + class-fixed snapshots Jul 11, 2023
@ovflowd
Copy link
Member Author

ovflowd commented Jul 11, 2023

But FYI @ljharb I'm more than open for suggestions; it just feels that at least, to the extent of my knowledge, this is what I've found to reduce the noise and issues we have.

Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

comments, mostly on the dependencies documentation

.storybook/test-runner.ts Show resolved Hide resolved
DEPENDENCY_PINNING.md Outdated Show resolved Hide resolved
DEPENDENCY_PINNING.md Outdated Show resolved Hide resolved
DEPENDENCY_PINNING.md Outdated Show resolved Hide resolved
DEPENDENCY_PINNING.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Co-authored-by: Brian Muenzenmeyer <[email protected]>
Signed-off-by: Claudio Wunder <[email protected]>
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 11, 2023 18:32 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 11, 2023 18:33 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 11, 2023 18:36 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 11, 2023 18:37 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 11, 2023 18:57 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 11, 2023 18:57 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 11, 2023 19:12 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 11, 2023 19:13 Inactive
@ovflowd ovflowd merged commit 13b859e into nodejs:main Jul 12, 2023
@ovflowd ovflowd deleted the chore/update-dependencies branch July 12, 2023 08:39
umairraza96 pushed a commit to umairraza96/nodejs.org that referenced this pull request Jul 12, 2023
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.

6 participants