Skip to content

fix(deps): downgrade preact#641

Merged
ovflowd merged 3 commits intomainfrom
fix/deps
Mar 4, 2026
Merged

fix(deps): downgrade preact#641
ovflowd merged 3 commits intomainfrom
fix/deps

Conversation

@avivkeller
Copy link
Member

@avivkeller avivkeller commented Mar 3, 2026

I've made two discoveries in the past few minutes:

  1. We have been using Preact's beta, which doesn't follow Semver, and makes Preact unupdatable

Requesting fast-track

Copilot AI review requested due to automatic review settings March 3, 2026 20:54
@avivkeller avivkeller requested a review from a team as a code owner March 3, 2026 20:54
@avivkeller avivkeller added the fast track This PR can land before the typical review time, with a :+1: from collaborators label Mar 3, 2026
@vercel
Copy link

vercel bot commented Mar 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
api-docs-tooling Ready Ready Preview Mar 4, 2026 1:24am

Request Review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses dependency stability by moving off a Preact beta release (which doesn’t follow semver reliably) and standardizing dependency locking with package-lock.json.

Changes:

  • Downgrade preact from ^11.0.0-beta.0 to the latest stable ^10.28.4.
  • Use/refresh package-lock.json to lock the dependency graph under the stable Preact version.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

File Description
package.json Switches preact dependency from v11 beta to stable v10.28.4.
package-lock.json Locks resolved dependencies to match the updated stable Preact dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Mar 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.87%. Comparing base (a41a0d3) to head (b7ce657).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #641   +/-   ##
=======================================
  Coverage   73.87%   73.87%           
=======================================
  Files         145      145           
  Lines       12867    12867           
  Branches      923      923           
=======================================
  Hits         9505     9505           
  Misses       3357     3357           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link

github-actions bot commented Mar 3, 2026

orama-db Generator

File Base Head Diff
orama-db.json 8.04 MB 8.04 MB -2.00 B (-0.00%)

web Generator

File Base Head Diff
styles.css 128.16 KB 128.22 KB +67.00 B (+0.05%)

Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

There is a really significant diff on the lockfile, including what looks to be a very large number of transitive dependency bumps which don't seem related to downgrading preact?

What is the history of why this uses shrinkwrap instead of a lockfile, as I imagine that was a very intentional decision, and I'd like to understand that before we move forward with undoing that decision?

@avivkeller
Copy link
Member Author

avivkeller commented Mar 3, 2026

There is a really significant diff on the lockfile, including what looks to be a very large number of transitive dependency bumps which don't seem related to downgrading preact?

Correct. The shrinkwrap and lockfile are different in how they function, so I had to regenerate the lockfile. All dependency changes, while they look unrelated, stay in the constraints we defined in the package.json.

What is the history of why this uses shrinkwrap instead of a lockfile, as I imagine that was a very intentional decision, and I'd like to understand that before we move forward with undoing that decision?

It was an intentional decisions, made at a time when @node-core/* didn't follow semver, and we needed any doc-kit consumers to use the exact hash of those core dependencies. Now, this is no longer the case, as those dependencies are published with regular X.Y.Z format.

@MattIPv4
Copy link
Member

MattIPv4 commented Mar 3, 2026

Correct. The shrinkwrap and lockfile are different in how they function, so I had to regenerate the lockfile. All dependency changes, while they look unrelated, stay in the constraints we defined in the package.json.

I'm not sure I follow this? You can look at the diff and clearly see the files are the same structure, you've just regenerated it and thus bumped all the transitive dependencies in the process -- while I agree it technically falls within bounds of the package.json definitions, I am quite hesitant to approve such a vast bump to all our transitive dependencies.

@MattIPv4
Copy link
Member

MattIPv4 commented Mar 3, 2026

Per https://docs.npmjs.com/cli/v11/configuring-npm/npm-shrinkwrap-json, shrinkwarp is exactly the same as lockfile other than that it gets published with the package, so I do not see why regeneration + bumping versions was needed, it should just be a case of renaming the file?

@avivkeller
Copy link
Member Author

I knew they were the same structure, however, I didn't realize they were interchangeable (i.e. manifest.json's in chrome extensions are all the same structure, but v2, v3, etc, cannot be interchanged).

Sorry.

@MattIPv4
Copy link
Member

MattIPv4 commented Mar 3, 2026

No worries :)

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

We should keep npm-shrinkwrap. We're not installing doc-kit via git. npm-shrinkwrao was a requirement done by @aduh95 -- I'd appreciate if we don't keep reverting previous decisions then doing fast-track on such quick decisions. Ask questions first, please (check git blame, for example, understand why certain decisions were made instead of quickly challenging them). Thanks!

@MattIPv4
Copy link
Member

MattIPv4 commented Mar 3, 2026

I'd appreciate if we don't keep reverting previous decisions then doing fast-track on such quick decisions. Ask questions first, please (check git blame, for example, understand why certain decisions were made instead of quickly challenging them). Thanks!

That was exactly what was done...

What is the history of why this uses shrinkwrap instead of a lockfile, as I imagine that was a very intentional decision, and I'd like to understand that before we move forward with undoing that decision?

It was an intentional decisions, made at a time when @node-core/* didn't follow semver, and we needed any doc-kit consumers to use the exact hash of those core dependencies. Now, this is no longer the case, as those dependencies are published with regular X.Y.Z format.

Based on that information, I've approved it as it seems fine to switch over to a regular lock now.

@ovflowd
Copy link
Member

ovflowd commented Mar 3, 2026

Based on that information, I've approved it as it seems fine to switch over to a regular lock now.

I'm unsure this is the reason behind, and I would please have @aduh95 to chime in here first before we merge this.

@ovflowd
Copy link
Member

ovflowd commented Mar 3, 2026

I'd appreciate if we don't keep reverting previous decisions then doing fast-track on such quick decisions. Ask questions first, please (check git blame, for example, understand why certain decisions were made instead of quickly challenging them). Thanks!

That was exactly what was done...

What is the history of why this uses shrinkwrap instead of a lockfile, as I imagine that was a very intentional decision, and I'd like to understand that before we move forward with undoing that decision?

It was an intentional decisions, made at a time when @node-core/* didn't follow semver, and we needed any doc-kit consumers to use the exact hash of those core dependencies. Now, this is no longer the case, as those dependencies are published with regular X.Y.Z format.

Based on that information, I've approved it as it seems fine to switch over to a regular lock now.

I didn't read the comments, sorry, I just quickly saw this PR and added a block to prevent it getting merged due to concerns I have. If Antoine gives a 👍 go for it, if not, please reach consensus 🙇

@avivkeller
Copy link
Member Author

Yes, of course, it's never a bad idea to get more approvals. Just to clarify on the fast track, the web generator will not run in node core without this PR, so I requested a fast track to unblock my testing (which is currently done by just using the commit on this PRs head)

@aduh95
Copy link
Contributor

aduh95 commented Mar 4, 2026

The requirement came from nodejs/node#57343 (comment), as the idea at the time was to use npx at the time. Is there any pressing reason to go back to package-lock?

@avivkeller
Copy link
Member Author

Is there any pressing reason to go back to package-lock?

Yes, the npm shrinkwrap isn't being respected, thus it serves no purpose.

@ovflowd
Copy link
Member

ovflowd commented Mar 4, 2026

Is there any pressing reason to go back to package-lock?

Yes, the npm shrinkwrap isn't being respected, thus it serves no purpose.

Can you please provide proof it is not being respected? And wdym by that?

@ovflowd
Copy link
Member

ovflowd commented Mar 4, 2026

Even if it is not, there is no reason to go to package-lock. Sorry, for me this is a no. Not going to withdraw my block till better info is provided here, sorry Aviv 😭😅

@avivkeller
Copy link
Member Author

Okay, I'll break this into two PRs. The preact downgrade is a direct blocker for moving core to the web generator, and I'll open a separate PR with more evidence as to switching is to the normal lock file

@avivkeller avivkeller changed the title fix(deps): downgrade preact + move to package-lock fix(deps): downgrade preact Mar 4, 2026
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

SGTM!

@ovflowd
Copy link
Member

ovflowd commented Mar 4, 2026

Okay, I'll break this into two PRs. The preact downgrade is a direct blocker for moving core to the web generator, and I'll open a separate PR with more evidence as to switching is to the normal lock file

I think that would have been the ideal from the go, btw!

@avivkeller
Copy link
Member Author

Yup

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGMT ! but for the fast-track wait Antoine to answer

@aduh95
Copy link
Contributor

aduh95 commented Mar 4, 2026

Is there any pressing reason to go back to package-lock?

Yes, the npm shrinkwrap isn't being respected, thus it serves no purpose.

I mean it serves the purpose of a lock file, switching to package-lock.json should be equivalent, no? That doesn't seem a good candidate for fast-tracking at the very least.

LGMT ! but for the fast-track wait Antoine to answer

I have no opinion on the Preact downgrade, if it's a blocker fast-tracking seems like the correct course of action.

@ovflowd ovflowd merged commit 0470a1c into main Mar 4, 2026
20 checks passed
@ovflowd ovflowd deleted the fix/deps branch March 4, 2026 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast track This PR can land before the typical review time, with a :+1: from collaborators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants