build: explore the feasibility of dropping ncc in favor of building with webpack directly #194
Conversation
yjoer
changed the title
yjoer
changed the title
yjoer
force-pushed
the
patch-1
branch
2 times, most recently
from
9cbcad2 to
ff83ab9
Compare
Extracted from #194 I aliased `node:path` to `path` within the `staticModules` object so that any encounters of `node:path` would have the same behavior as `path`. I have added unit tests specifically for `path` and `node:path`. Additionally, I've included a new chunk in the integration test to ensure asset references work correctly when using `resolve` or `path.resolve` imported from `node:path`. - Fixes: #191
…#198) Extracted from #194 This is taking advantage of #185, which used a function in node-gyp-build to compute the path. Some moons ago, [node-gyp-build.js#L62-L75](https://github.com/prebuild/node-gyp-build/blob/v4.8.4/node-gyp-build.js#L62-L75) was updated with a function to resolve native addons from the prebuilds directory. All we have to do here is update `node-gyp-build`. This enables assets relocation for packages like [bcrypt v6.0.0](https://www.npmjs.com/package/bcrypt?activeTab=code) I added some tests according to the platform used in CI. Hopefully, the architecture and the fixtures align well. --------- Co-authored-by: Steven <[email protected]>
yjoer
changed the title
styfle
left a comment
styfle
left a comment
There was a problem hiding this comment.
This should also update gh actions workflow to run the tests, right?
|
Uh, yes. The conservative way is to test this in addition to existing tests based on ncc builds. If it doesn't introduce issues when making other changes for some time, we can promote this as the primary way. What do you think? |
|
I'm also wondering if we need to bundle this at all. Since its a plugin, it seems strange that we bundle before publishing to npm. Usually thats for CLIs or frontend code, but not a plugin like this. |
|
The benefit of bundling is that it eliminates the need to install dependencies and numerous of their descendants, which are now listed under dev dependencies. The tradeoff, however, is that it makes the package harder to read and inspect. Back at it again, trying to choose between two paths 😵 |
|
I moved the runtime dependencies into a temporary file package.json and visualized the dependency graph. It appears that node-pre-gyp is the only package responsible for introducing a large number of transitive dependencies.
|
|
We could swap it for the |
|
🎉 This issue has been resolved in version 1.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I replaced node-pre-gyp with the latest version, shrinking the [dependency graph](https://npmgraph.js.org/?q=%40vercel%2Fwebpack-asset-relocator-loader%400.0.0-development#packages=%5B%7B%22dependencies%22%3A%7B%22%40mapbox%2Fnode-pre-gyp%22%3A%22%5E2.0.0%22%2C%22acorn%22%3A%22%5E8.3.0%22%2C%22acorn-class-fields%22%3A%22%5E1.0.0%22%2C%22acorn-private-class-elements%22%3A%22%5E1.0.0%22%2C%22acorn-static-class-features%22%3A%22%5E1.0.0%22%2C%22bindings%22%3A%22%5E1.4.0%22%2C%22estree-walker%22%3A%22%5E0.6.1%22%2C%22glob%22%3A%22%5E7.1.3%22%2C%22graceful-fs%22%3A%22%5E4.1.15%22%2C%22loader-utils%22%3A%22%5E1.2.3%22%2C%22magic-string%22%3A%22%5E0.25.1%22%2C%22node-gyp-build%22%3A%22%5E4.8.4%22%2C%22resolve%22%3A%22%5E1.10.0%22%2C%22resolve-from%22%3A%223.0.0%22%2C%22rollup-pluginutils%22%3A%22%5E2.8.2%22%2C%22sourcemap-codec%22%3A%22%5E1.4.4%22%7D%2C%22name%22%3A%22%40vercel%2Fwebpack-asset-relocator-loader%22%2C%22version%22%3A%220.0.0-development%22%7D%5D) from 95 to 56 packages. Since both the regular and coverage test modes now load the loader from source, output discrepancies between them have been eliminated. Additionally, I've added integration tests for both node-pre-gyp and node-gyp-build, using bcrypt versions 5 and 6. The split chunk logic in the test project has also been updated so that each entry point now generates its own vendor chunk. Closes vercel#194
2 participants
Now that the dust has settled, I'm repurposing this PR to focus on simplifying the build process by removing the dependency on ncc and replacing it with a direct webpack build. This change helps resolve the cyclic dependency and avoids some of the quirks associated with ncc. The tradeoff is that we now need to maintain a small custom webpack configuration, and there's a risk that certain edge cases previously handled by ncc may not be fully covered.
Building the loader with webpack passes the same set of tests as the build script. This brings us to a decision point: either continue using ncc or move forward without it. This, perhaps, provides a fallback in case ncc introduces issues again.
I'm working on fixing the tests for #193. While running them, the only intermittent failure I encountered was in
project.test.js, where the number of lines in the webpack output is being asserted. After running the tests multiple times, I observed that the output fluctuates between 16 and 17 lines. To address this, we could update the test to allow either value.16 lines:
17 lines: