Skip to content
This repository was archived by the owner on Dec 5, 2019. It is now read-only.

fix(index): ensure to nullify inputSourceMap if sourcemap is unavailable (devtool) #169

Merged
merged 1 commit into from
Nov 23, 2017
Merged

fix(index): ensure to nullify inputSourceMap if sourcemap is unavailable (devtool) #169

merged 1 commit into from
Nov 23, 2017

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Nov 20, 2017 •
edited by michael-ciniawsky
Loading

joshwiens
joshwiens reviewed Nov 20, 2017
View reviewed changes
test/__snapshots__/source-map-options.test.js.snap Outdated
],
"sources": Array [
"/home/evilebottnawi/IdeaProjects/uglifyjs-webpack-plugin/test/fixtures/entry.js",
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always fail somewhere else with an absolute file path

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@d3viant0ne fixed

@alexander-akait alexander-akait force-pushed the issue-111 branch 2 times, most recently from 6af0832 to 75b0138 Compare November 20, 2017 13:20
michael-ciniawsky
michael-ciniawsky suggested changes Nov 20, 2017
View reviewed changes
src/index.js Outdated
input = asset.source();
}
if (this.options.sourceMap && asset.sourceAndMap) {
const sourceAndMap = asset.sourceAndMap();
Copy link
Member

@michael-ciniawsky michael-ciniawsky Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const { source, map } = asset.sourceAndMap();

input = source;
inputSourceMap = map;

sourceMap = new SourceMapConsumer(inputSourceMap);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does webpack >= v2.0.0 already support this convenience method ? So the else branch is really legacy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-ciniawsky need investigate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be safe https://github.com/webpack/webpack/blob/webpack-2/package.json#L26

src/index.js Outdated
sourceMap = new SourceMapConsumer(inputSourceMap);
} else {
input = asset.source();
inputSourceMap = null;
Copy link
Member

@michael-ciniawsky michael-ciniawsky Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the [windows related] fix ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-ciniawsky no, when devtool set to null we don't have map

Copy link
Member

@michael-ciniawsky michael-ciniawsky Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evilebottnawi Sure, but this is nevertheless the fix for the parallel source maps bug :) ? It doesn't seem related to options.parallel exclusively then, should we better reframe the commit message ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-ciniawsky in this commit i just simplify logic and add tests to make sure that everything works and the problem is not in the plugin

@michael-ciniawsky michael-ciniawsky changed the title fix: source maps generation when use parallel option fix: parallel source maps generation (options.parallel) Nov 20, 2017
@michael-ciniawsky michael-ciniawsky changed the title fix: parallel source maps generation (options.parallel) fix(index): ensure to nullify inputSourceMap if sourcemap is unavailable (devtool) Nov 20, 2017
@michael-ciniawsky
Copy link
Member

@evilebottnawi Status 😛 ? Could you address the style nit, so we can get this in or is anything blocking this PR ?

@michael-ciniawsky michael-ciniawsky added this to the 1.1.1 milestone Nov 23, 2017
@alexander-akait
Copy link
Member Author

@michael-ciniawsky done 👍

michael-ciniawsky
michael-ciniawsky approved these changes Nov 23, 2017
View reviewed changes
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx

@michael-ciniawsky michael-ciniawsky merged commit 062ec72 into master Nov 23, 2017
@michael-ciniawsky michael-ciniawsky deleted the issue-111 branch November 23, 2017 16:34
@michael-ciniawsky michael-ciniawsky removed this from the 1.1.1 milestone Nov 23, 2017
@dependencies dependencies bot mentioned this pull request Nov 24, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joshwiens joshwiens joshwiens left review comments

@michael-ciniawsky michael-ciniawsky michael-ciniawsky approved these changes

Assignees
No one assigned
Labels
semver: Patch status: Approved type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexander-akait @michael-ciniawsky @joshwiens