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

Fix sources: ["0"], output the correct source #140

Merged
merged 2 commits into from
Apr 16, 2019

Conversation

xg-wang
Copy link
Member

@xg-wang xg-wang commented Apr 12, 2019

Case 1, sourcemap missing
In terser-js, sources are always 0 if old sourcemaps are not provided.

The value passed for sourceMap.url is only used to set //# sourceMappingURL=out.js.map in result.code. The value of filename is only used to set file attribute (see the spec) in source map file.

In broccoli-uglify-sourcemap we know in this case we are generating sourcemap for the file we are processing, changing 0 to the actual file gives us the correct source.

Case 2, multiple sourceMap comments
source-map-url only matches the very first magic comment, that let us mistakenly thinks the js file doesn't have a valid sourcemap.

Fix #130

@xg-wang xg-wang changed the title Fix sources: ["0"], output the correct source WIP Fix sources: ["0"], output the correct source Apr 12, 2019
Case 1, sourcemap missing
In [terser-js](https://github.com/terser-js/terser#source-map-options),
sources are always 0 if old sourcemaps are not provided.

The value passed for sourceMap.url is only used to set
//# sourceMappingURL=out.js.map in result.code.

The value of filename is only used to set file attribute
in source map file.

In broccoli-uglify-sourcemap we know in this case we are generating
sourcemap for the file we are processing, changing 0 to the actual
file gives us the correct source.

Case2, multiple sourceMap comments
source-map-url only matches the very first magic comment so we
mistakenly thinks the js file doesn't have a valid sourcemap.
@xg-wang xg-wang changed the title WIP Fix sources: ["0"], output the correct source Fix sources: ["0"], output the correct source Apr 13, 2019
@xg-wang
Copy link
Member Author

xg-wang commented Apr 13, 2019

These are 2 cases I met in some apps where sorucemaps have sources: [0], which makes reading the original file difficult.
There might be another issue from broccoli-concat where it leaves multiple magic comments in a js file.

Should supersede #58

@xg-wang
Copy link
Member Author

xg-wang commented Apr 13, 2019

Need review @stefanpenner @Turbo87

lib/get-sourcemap-content.js Outdated Show resolved Hide resolved
lib/process-file.js Outdated Show resolved Hide resolved
@xg-wang
Copy link
Member Author

xg-wang commented Apr 16, 2019

As suggested by @stefanpenner I created 2 benchmarks comparing

The result on my mac:

  while ((matchedUrl = srcURL.getFrom(src)) !== null) {
    urls.push(matchedUrl);
    src = srcURL.removeFrom(src);
  }

run.js n=100 relativePath="ember.prod.js": 25.288791793887814
run.js n=100 relativePath="ember.prod.bad.js": 35.53592907140567

  while (match = srcRegExpg.exec(src)) {
    urls.push(match[1] || match[2] || '');
  }

run.js n=100 relativePath="ember.prod.js": 26.578133126260052
run.js n=100 relativePath="ember.prod.bad.js": 1,097.2937083164868

So add another commit here to use getFrom and removeFrom seems more reasonable.

@stefanpenner stefanpenner merged commit 7183703 into ember-cli:master Apr 16, 2019
@stefanpenner
Copy link
Collaborator

released as v3.1.1 🎉

@robclancy
Copy link

Just spent so long thinking I had configured something wrong trying to sort this out. And turns out a fix has been here all along :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sources:["0"] in no-upstream and with-broken sourcemap
3 participants