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

chore: Fix top-level dependency install for node 22+ #6658

Merged

Conversation

benjamincburns
Copy link
Contributor

@benjamincburns benjamincburns commented Aug 29, 2024

Prior to this change, running yarn in the top-level directory of the repo failed on node >= v22. After this change, it succeeds.

This was cased by the version of the better-sqlite3 package being pinned to 9.4.0 in the top-level package.json. This change was introduced by @jacoblee93 in #5694. It's not clear from that PR why he added the package to the top-level package.json (I can't find anything that references it in the top-level), or why he pinned it to that specific version. Perhaps he could chime in here.

Edit: I updated this change to remove the range definition from the top-level resolutions object, as my initial change would've had the same effect, but with the caveat that it'd need to be maintained in step with the version of better-sqlite3 defined in the community package.

Rather than removing it, my change aligns the version range with one in the @langchain/community package (listed here as a devDependency and here as an optional peerDependency), which is the only other package in the repo that depends on better-sqlite3.

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Aug 29, 2024
Copy link

vercel bot commented Aug 29, 2024

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

Name Status Preview Comments Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Sep 4, 2024 8:19pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ⬜️ Ignored (Inspect) Sep 4, 2024 8:19pm

@dosubot dosubot bot added the auto:bug Related to a bug, vulnerability, unexpected error with an existing feature label Aug 29, 2024
Copy link
Member

@bracesproul bracesproul left a comment

Choose a reason for hiding this comment

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

The dependency is in the resolutions field, which means it's not actually a top level dependency, but rather it'll force all workspaces which have better-sqlite3 as a dependency to use that specified version.

I think if this PR passes CI it should be good to go since resolutions in this instance is for development and not released code, but cc @jacoblee93 incase I'm missing something.

@benjamincburns
Copy link
Contributor Author

Damn @bracesproul you're quick today! Thanks for chiming in!

@benjamincburns
Copy link
Contributor Author

@bracesproul on further consideration, my change is effectively rendering the better-sqlite3 entry in the resolutions object moot, right? so it's effectively the same as removing it? In that case I'll revise this PR to just remove it, as there's no need to have the extra cruft hanging around.

@bracesproul
Copy link
Member

bracesproul commented Aug 29, 2024

@bracesproul on further consideration, my change is effectively rendering the better-sqlite3 entry in the resolutions object moot, right? so it's effectively the same as removing it? In that case I'll revise this PR to just remove it, as there's no need to have the extra cruft hanging around.

No, resolutions will also override any other dependencies which have a dependency on better-sqlite3. So this could be fixing a bug caused by another dependency depending on a different version of better-sqlite3.

@benjamincburns
Copy link
Contributor Author

benjamincburns commented Aug 29, 2024

Ah, I see - and sorry for the confusion on my part. I'm quite familiar with node package management via npm, but I haven't used yarn in around 5 years or so, and I don't think I've ever used yarn 2.

So this could be fixing a bug caused by another dependency depending on a different version of better-sqlite3.

If what you said above is true, that the top-level resolutions config only applies for local dev/test purposes (makes sense, as the top-level package is private), wouldn't it be problematic to apply a resolution in this way to "fix" whatever bug that might be? That is, the published package will allow a peer better-sqlite3 to be any version in >=9.4.0 <12. If there was a bug that occurred as a result of using some version in that range, wouldn't pinning the package to 9.4.0 via a dev-only override in this way make it so that the bug is only visible after release? If it were me, I'd want it to be caught and fixed during my local dev/test cycles, and/or by CI test runs.

Assuming that this is considering the full dependency tree (command ran from repo root), I don't see any usages of better-sqlite3 that would fall outside of the range in my initial commit (>=9.4.0 <12). The only version that I can see in use there is the latest one, 11.2.1. Note that this was gathered without an override for better-sqlite3 listed in the top-level package.json resolutions object.

$ yarn why --recursive --peers better-sqlite3
├─ @langchain/community@workspace:libs/langchain-community
│  ├─ better-sqlite3@npm:11.2.1 (via npm:>=9.4.0 <12.0.0)
│  ├─ @getzep/zep-cloud@npm:1.0.6 [0e7b0] (via npm:^1.0.6 [0e7b0])
│  │  └─ langchain@workspace:langchain [0e7b0] (via ~0.2.3 [0e7b0])
│  ├─ @langchain/langgraph@npm:0.0.34 [0e7b0] (via npm:~0.0.26 [0e7b0])
│  │  └─ better-sqlite3@npm:11.2.1 (via npm:>=9.4.0 <12.0.0)
│  ├─ langchain@workspace:langchain [0e7b0] (via ~0.2.3 [0e7b0])
│  ├─ langsmith@npm:0.1.39 [0e7b0] (via npm:~0.1.30 [0e7b0])
│  │  └─ langchain@workspace:langchain [0e7b0] (via ~0.2.3 [0e7b0])
│  └─ typeorm@npm:0.3.20 [0e7b0] (via npm:^0.3.20 [0e7b0])
│     └─ better-sqlite3@npm:11.2.1 (via npm:>=9.4.0 <12.0.0)
│
└─ examples@workspace:examples
   ├─ @getzep/zep-cloud@npm:1.0.6 [773c0] (via npm:^1.0.6 [773c0])
   │  └─ langchain@workspace:langchain [773c0] (via workspace:* [773c0])
   ├─ @langchain/community@workspace:libs/langchain-community [773c0] (via workspace:* [773c0])
   ├─ langchain@workspace:langchain [773c0] (via workspace:* [773c0])
   └─ langsmith@npm:0.1.43 [773c0] (via npm:^0.1.43 [773c0])
      └─ langchain@workspace:langchain [773c0] (via workspace:* [773c0])

@jacoblee93
Copy link
Collaborator

Thanks @benjamincburns, will look into this.

Copy link

vercel bot commented Sep 4, 2024

@jacoblee93 is attempting to deploy a commit to the LangChain Team on Vercel.

A member of the Team first needs to authorize it.

@jacoblee93
Copy link
Collaborator

Thanks again @benjamincburns, yeah I think my initial PR was a quick fix to an acute problem (conflicting peer deps on npm install @langchain/community. I think moving the resolutions to devdeps is fine as I've done.

Thanks for flagging!

@jacoblee93 jacoblee93 merged commit 90dfda5 into langchain-ai:main Sep 4, 2024
34 checks passed
@jacoblee93 jacoblee93 changed the title Fix top-level dependency install for node 22+ chore:Fix top-level dependency install for node 22+ Sep 4, 2024
@jacoblee93 jacoblee93 changed the title chore:Fix top-level dependency install for node 22+ chore: Fix top-level dependency install for node 22+ Sep 4, 2024
@benjamincburns
Copy link
Contributor Author

benjamincburns commented Sep 5, 2024

@jacoblee93 thanks! In case you missed it, however - better-sqlite3 didn't officially support node v22 until v10.0.0. It looks like you've pinned to v9.5.0. I pulled latest main after this change was merged and it does successfully build on my machine (running node v22.2.0 under Ubuntu via WSL), but I thought I'd just mention it anyway just in case there were other changes between v9.5.0 and v10.0.0 that were necessary for proper node v22 support.

FWIW, WiseLibs/better-sqlite3#1182 is the PR where they implemented support. It looks like it was just changes to their underlying build config - not seeing any logic changes, anyway. It's probably fine to leave as-is with v9.5.0, but I haven't looked thoroughly enough to say that with confidence.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto:bug Related to a bug, vulnerability, unexpected error with an existing feature size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants