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

Improve bundling for hoisted dependencies #14708

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

msujew
Copy link
Member

@msujew msujew commented Jan 8, 2025

What it does

Closes #14707

This change just sets the paths field whenever using require.resolve to correctly find the hoisted dependencies from the point they are imported.

Note that we have a reference to node-pty in our generated webpack config that cannot be refactored to use require.resolve. If this dependency is not hoisted, the webpack build will fail with a helpful error message.

How to test

Use #14481 and replace "node_modules/trash" with "packages/filesystem/node_modules/trash" in your package-lock.json file. Same with other dependencies such as @vscode/ripgrep, drivelist or @parcel/watcher.

Review checklist

Reminder for reviewers

@msujew msujew added the bundle Related to webpack bundling label Jan 8, 2025
@JonasHelming JonasHelming requested a review from sdirix January 29, 2025 09:28
@sdirix
Copy link
Member

sdirix commented Jan 29, 2025

@msujew can you fix the lint issues?

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

I tested by

  • merging with latest master state
  • replacing
    • node_modules/drivelist with packages/core/node_modules/drivelist
    • node_modules/drivelist/node_modules/node-addon-api with packages/core/node_modules/drivelist/node_modules/node-addon-api
    • node_modules/trash with packages/filesystem/node_modules/trash
  • building and running the electron app

Seems to work!

I also tested

  • replacing node_modules/node-pty with packages/process/node_modules/node-pty
  • building the electron app

Fails as expected!

@msujew msujew merged commit 3f05454 into master Jan 29, 2025
11 checks passed
@msujew msujew deleted the msujew/require-resolve-paths branch January 29, 2025 17:33
@github-actions github-actions bot added this to the 1.58.0 milestone Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bundle Related to webpack bundling
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Make NativeWebpackPlugin resilient to Hoisting
2 participants