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

fix: install vendored node_modules when using hardlinks #9007

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

eemelipa
Copy link
Contributor

@eemelipa eemelipa commented Feb 20, 2024

What does this PR do?

Fixes issue where vendored node_modules aren't installed with hardlink backend.

Vendored node_module is a folder inside an installed package which is called node_module. So the library has bundled some of their dependencies into the published package instead of specifying it in their package.json. For example

An npm package is called vendor-baz and it contains the following files:

x index.js
x package.json
x cjs/
x cjs/node_modules/
x cjs/node_modules/foo-dep/
x cjs/node_modules/foo-dep/index.js

The contents of cjs/node_modules/foo-dep/ is a vendored package.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I wrote automated tests

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

beforeEach(dummyBeforeEach);
afterEach(dummyAfterEach);

it("should install vendored node_modules with hardlink", async () => {
Copy link
Contributor Author

@eemelipa eemelipa Feb 20, 2024

Choose a reason for hiding this comment

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

Similar testing approach as tests in test/cli/install/bun-install.test.ts and more specifically test case should prefer latest-tagged dependency

@eemelipa eemelipa marked this pull request as ready for review February 20, 2024 04:08
@eemelipa eemelipa changed the title fix: install vendored node_modules when using hardlinks (#8093) fix: install vendored node_modules when using hardlinks Feb 20, 2024
@@ -1366,7 +1366,7 @@ pub const PackageInstall = struct {
cached_package_dir,
this.allocator,
&[_]bun.OSPathSlice{},
&[_]bun.OSPathSlice{bun.OSPathLiteral("node_modules")},
Copy link
Member

Choose a reason for hiding this comment

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

TIL you can publish an npm package with a node_modules folder as long as it isn't top level

Copy link
Member

@dylan-conway dylan-conway left a comment

Choose a reason for hiding this comment

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

Looks good to me. Will wait for CI to run

@dylan-conway
Copy link
Member

We probably also want to do this for installWithSymlink, but it can be addressed in a future pr. I'm guessing there's a good chance top level node_modules will actually exist for local packages, so we would need to skip it

@dylan-conway dylan-conway merged commit 48e7c0f into oven-sh:main Feb 20, 2024
28 of 31 checks passed
@dylan-conway
Copy link
Member

Thank you

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.

Installed package missing vendored node_modules directory
2 participants