Skip to content

First stab at SourceMapDevToolPlugin exclude option - #897

Merged
sokra merged 3 commits into
webpack:masterfrom
sebastienbarre:seb/650
Mar 28, 2015
Merged

First stab at SourceMapDevToolPlugin exclude option#897
sokra merged 3 commits into
webpack:masterfrom
sebastienbarre:seb/650

Conversation

@sebastienbarre

@sebastienbarre sebastienbarre commented Mar 17, 2015

Copy link
Copy Markdown
Contributor

per @gaearon suggestion, see issue #650 raised by @davidtheclark: "The problem with this is that the vendor file can get pretty big, and its gigantic sourcemap then severely slows down dev tool initialization on page reload. I would much prefer to only have a sourcemap for my app chunk (and not to have a sourcemap for my vendor chunk)."

This is a first stab at the corresponding exclude options.
Note that it might get tedious to have to explicitly instantiate SourceMapDevToolPlugin, maybe a more global option such as output.sourceMapExcludeChunks could be introduced as well.

    plugins: [
        new webpack.SourceMapDevToolPlugin({
            filename: "[file].map",
            excludeChunks: ["vendors"]
        })

@sokra

sokra commented Mar 19, 2015

Copy link
Copy Markdown
Member

Please make it behave like the UglifyJsPlugin which has similar options: test include exclude https://github.com/webpack/webpack/blob/master/lib/optimize/UglifyJsPlugin.js#L43

@sebastienbarre

sebastienbarre commented Mar 19, 2015

Copy link
Copy Markdown
Contributor Author

@sokra ha, I didn't know about these options, there is not much documentation for include or exclude on UglifyJsPlugin, neither was I able to find tests stressing out these features. Nevertheless, here is the revised version.

  1. The way SourceMapDevToolPlugin handles options looks very different than the way UglifyJsPlugin does it; the former copies the options back to member variables (this.cheapMode = options.cheapMode) while the latter is just content with assigning them to an object of the same name (this.options = options). I didn't want to completely break SourceMapDevToolPlugin so I didn't rewrite that behavior, I just made sure a this.options var now exists, so that it can be passed to SourceMapDevToolPlugin.prototype.matchObject.
  2. I've only added the exclude option. I can easily put the test and include options back, just like UglifyJsPlugin, but I'm not sure how this would make sense. Let me know.

@sebastienbarre sebastienbarre changed the title First stab at SourceMapDevToolPlugin excludeChunks option First stab at SourceMapDevToolPlugin exclude option Mar 19, 2015
sokra
sokra reviewed Mar 20, 2015
View reviewed changes
Comment thread lib/SourceMapDevToolPlugin.js Outdated

@sokra sokra Mar 20, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you move this into a separate file? So it's shared by UglifyJsPlugin and SourceMapDevToolPlugin. We don't want to have duplicate code.

@sebastienbarre sebastienbarre Mar 20, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Since one of them was already using ModuleFilenameHelpers and this helper seemed to deal with files already, that's where I moved these functions, but I could move them to yet another file if you suggest a name.

@sokra

sokra commented Mar 20, 2015

Copy link
Copy Markdown
Member

I've only added the exclude option. I can easily put the test and include options back, just like UglifyJsPlugin, but I'm not sure how this would make sense. Let me know.

Yes please add them back. (Both plugins should share the same logic)

@sokra

sokra commented Mar 20, 2015

Copy link
Copy Markdown
Member

The way SourceMapDevToolPlugin handles options looks very different than the way UglifyJsPlugin does it; the former copies the options back to member variables (this.cheapMode = options.cheapMode) while the latter is just content with assigning them to an object of the same name (this.options = options). I didn't want to completely break SourceMapDevToolPlugin so I didn't rewrite that behavior, I just made sure a this.options var now exists, so that it can be passed to SourceMapDevToolPlugin.prototype.matchObject.

Previously the SourceMapDevToolPlugin didn't use a options object. This was refactored latter.

@sebastienbarre

sebastienbarre commented Mar 20, 2015

Copy link
Copy Markdown
Contributor Author

Alright, here you go, factorized code, which brings back include and test in the process.

@sebastienbarre

sebastienbarre commented Mar 26, 2015

Copy link
Copy Markdown
Contributor Author

Anything else you want me to add?

@sokra

sokra commented Mar 26, 2015

Copy link
Copy Markdown
Member

Anything else you want me to add?

No looks good... I'll merge it once I have some free time...

@sebastienbarre

sebastienbarre commented Mar 26, 2015

Copy link
Copy Markdown
Contributor Author

Thanks, was just checking. Keep up the good work.

sokra added a commit that referenced this pull request Mar 28, 2015
@sokra
Merge pull request #897 from sebastienbarre/seb/650
f7445f6 
First stab at SourceMapDevToolPlugin exclude option
@sokra
sokra merged commit f7445f6 into webpack:master Mar 28, 2015
@sokra

sokra commented Mar 28, 2015

Copy link
Copy Markdown
Member

Thanks

@sebastienbarre
sebastienbarre deleted the seb/650 branch March 28, 2015 16:52
@sebastienbarre sebastienbarre mentioned this pull request Apr 8, 2015
Closed
@sokra sokra mentioned this pull request Apr 22, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sebastienbarre @sokra