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

ncc 0.33 outputs source map twice #827

Open
onigoetz opened this issue Dec 3, 2021 · 7 comments
Open

ncc 0.33 outputs source map twice #827

onigoetz opened this issue Dec 3, 2021 · 7 comments
Labels
bug Something isn't working

Comments

@onigoetz
Copy link

onigoetz commented Dec 3, 2021

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 : 0.31.1...0.32.0
My bet would be something in between Webpack 5.44.0 and 5.62.1

@styfle
Copy link
Member

styfle commented Dec 4, 2021

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

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

@styfle styfle added the bug Something isn't working label Dec 4, 2021
@onigoetz
Copy link
Author

onigoetz commented Dec 4, 2021

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
Copy link
Member

styfle commented Dec 6, 2021

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

Thanks for looking into it 👍

@onigoetz
Copy link
Author

onigoetz commented Dec 7, 2021

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
Copy link
Member

styfle commented Dec 8, 2021

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
Copy link
Author

onigoetz commented Dec 8, 2021

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
Copy link

abcfy2 commented Dec 10, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants