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.17k stars 287 forks source link

ncc 0.33 outputs source map twice #827

Open onigoetz opened 2 years ago

onigoetz commented 2 years ago

Sourcemaps have become weirder and weirder from 0.31 to 0.32 and then 0.33.

0.31.1

Giving an entry file of babel-packages.js would emit a sourcemap and add the comment //# sourceMappingURL=babel-packages.js.map at the end of the file.

0.32.0

For the same input, emits a sourcemap and adds the comment //# sourceMappingURL=index.js.map no matter what the input file name is

0.33.0

For the same input, emits two sourcemaps and adds the comment //# sourceMappingURL=index.js.map. One is emitted as an asset, one in the map argument. ( most probably related to that change : https://github.com/vercel/ncc/pull/818/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R504-R507 )

This is problematic because I might have more than one bundle and if they all have index.js.map that's not gonna work :/

Here is the script I'm using :

https://github.com/swissquote/crafty/blob/master/utils/compile.js#L19-L49

And the output with 0.33.0

Writing dist/compiled/babel-packages.js 1.35 MB
Writing dist/compiled/index1.js 10.04 KB
Writing dist/compiled/index2.js 1.82 KB
Writing dist/compiled/index.js.map 1.21 MB
Writing dist/compiled/sourcemap-register.js 39.73 KB
Writing dist/compiled/babel-packages.js.map 1.21 MB
Writing dist/compiled/babel-packages-stats.json 3.89 MB

As you can see the two lines Writing dist/compiled/index.js.map 1.21 MB and Writing dist/compiled/babel-packages.js.map 1.21 MB have the same size and are in fact identical.

The initial change was introduced somewhere between 0.31.1 to 0.32.0 : https://github.com/vercel/ncc/compare/0.31.1...0.32.0 My bet would be something in between Webpack 5.44.0 and 5.62.1

styfle commented 2 years ago

Sounds like it could be related to #818 cc @fenix20113

Feel free to submit a PR with a test if you have a fix 👍

onigoetz commented 2 years ago

I'm not sure which issue you're referring to since you're mentioning the issue itself.

In all cases I'll have a look if I can find a way to fix this and make a PR :)

styfle commented 2 years ago

Oops, I must have pasted the wrong ID. That was supposed to be #818 😅

Thanks for looking into it 👍

onigoetz commented 2 years ago

Ah yes that's the PR I linked to in the initial post :)

I made some investigation and here are my findings. I don't know what kind of PR this would need so I'm putting this here first to discuss.

Changes between 0.31.1 -> 0.32.0

There was indeed a change in how webpack handled sourcemaps, and it was in fact a bugfix. ncc has a filename option (that I didn't know of, more on this in a bit) that informs webpack of the name of the file that has to be transformed.

It seems that in 0.31.1, webpack ignored that information and simply took the entry's filename

Changes between 0.32.0 -> 0.33.0

Most probably because of this fix, PR #818 was created and introduced this specific change : https://github.com/vercel/ncc/pull/818/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556L503-R512

Which causes the sourcemap being set to the map key and inside assets at the same time :

ncc("myinput.js", {}).then(async ({ code, map, assets }) => {

  // both values are identical
  map === assets['index.js.map'].source

});

It is however possible to set a filename option that will change the output :

ncc("myinput.js", {
  filename: "somefile.js"
}).then(async ({ code, map, assets }) => {

  // both values are identical
  map === assets['somefile.js.map'].source

});

By setting the filename I was able to get the result I wanted to obtain initially

Conclusion and possible actions

So in the end there is nothing wrong per-se. but There are is something that is quite surprising and I wouldn't be surprised if other people are surprised by that.

Filename always defaults to index.(originalFilenameExtension). While that seems logical for the CLI, it's quite surprising to have a code and map entries in the object returned by the promise. Wouldn't it make more sense to move the content of code directly in assets like it was done for map in PR #818 ?

Having a separate key for code and map makes us assume that we can give them any file name but they are strongly linked by the sourcemap comment in the file, which is defined by the filename prop.

Another possibility is to automatically set the filename by the input filename if it's not already set.

What do you think ?

styfle commented 2 years ago

Hmm, I typically use the CLI, not the programatic API and I don't use source maps often, so I haven't seen this issue yet.

Another possibility is to automatically set the filename by the input filename if it's not already set.

I think the reason why dist/index.js is default is so that you can do node dist to run as well as require('dist') when bundling a dependency like we do with some Next.js dependencies.

@guybedford @ijjk @javivelasco Any thoughts here?

onigoetz commented 2 years ago

I think the reason why dist/index.js is default is so that you can do node dist to run as well as require('dist') when bundling a dependency like we do with some Next.js dependencies.

Yes I understand and completely agree with that reasoning for the CLI, but when using the programmatic API you're given a variable and have the role of writing that to a file, thus giving you a false sense of control on the name of that file, but if you are using sourcemaps you actually have to write it to index.js (if no filename is set) which, at least for the programmatic API is not obvious

abcfy2 commented 2 years ago

Yes, I have the same issue:

$ npm run build
> ncc build -sdm src/index.ts

ncc: Version 0.33.0
ncc: Compiling file index.js into CJS
ncc: Using typescript@4.5.3 (local user-provided)
Emitting /home/fengyu/projects/ncc-test/assets/test.txt for static use in module /home/fengyu/projects/ncc-test/src/index.ts
 0kB  dist/test.txt
 1kB  dist/index.js
 4kB  dist/index.js.map
 4kB  dist/index.js.map
40kB  dist/sourcemap-register.js
45kB  [1152ms] - ncc 0.33.0

index.js.map output twice.