vercel / ncc

Compile a Node.js project into a single file. Supports TypeScript, binary addons, dynamic requires.
https://npmjs.com/@vercel/ncc
MIT License
9.27k stars 291 forks source link

Bug: asset relocation doesn't work with yarn monorepos because `filterAssetBase` is set to `cwd` #951

Open rprovodenko opened 2 years ago

rprovodenko commented 2 years ago

Steps to reproduce:

Expected result:

Actual result:

My workaround:

Proposed solution:

I can create a PR that would address these two issues if I get the green light from you (the maintainers)

styfle commented 2 years ago

The filterAssetBase should be set to the root of the monorepo, not /, or else you might accidentally include files outside your monorepo. So if you run ncc build packages/a/index.js the cwd will be the correct value.

styfle commented 2 years ago

There are many options exposed by ncc in javascript but not via the cli command.

Which options are missing?

rprovodenko commented 2 years ago

Which options are missing?

Out of the ones that are shown here: https://github.com/vercel/ncc/blob/a99ca13f4105858e52091c6ccafca540c74f181d/readme.md#programmatically-from-nodejs

Also, it would probably be a good idea to allow configuring these properties as well https://github.com/vercel/webpack-asset-relocator-loader#usage-1

else you might accidentally include files outside your monorepo

I don't think it's the job of the builder to tell me where I want to include files from. If I wrote some code that is referencing some location outside of the monorepo then that location should be used.

If you want ncc to be "helpful" it should error instead - I don't want to debug why some asset is not being included (especially considering that ncc is not very good at relocating assets - it could be any host of reasons). You mention in your documentation that asset relocation doesn't always work and then add another way why it wouldn't work - without providing any error message or warning or even a sentence in the docs to mention this. This is definitely a time waster for consumers (we have wasted many hours on different occasions on debugging ncc's asset relocation). It goes against the principle of least surprise.

Plus, if you want to be "correct" then ncc should not allow assets from outside of the asset referencing package (not just outside of the monorepo), as obviously referencing an asset from another package is not correct.

Anyway, I don't think it should be the job of the builder to decide what I can and can't reference.

But, choosing between cwd and root level package - then let's at least set it to root level package

rprovodenko commented 2 years ago

Update: My proposed workaround (changing cwd to root level package before invoking build) doesn't work if you have paths defined in tsconfig (see https://github.com/vercel/ncc/issues/953)

rprovodenko commented 2 years ago

fix ready for review https://github.com/vercel/ncc/pull/955