fix(index): don't remove legal comments by default (options.extractComments)
#250
Conversation
| @@ -85,7 +122,7 @@ describe('when options.extractComments', () => { | |||
| source: () => '// Comment\nvar foo = 1;', | |||
| }, | |||
| 'test1.js': { | |||
| source: () => '/* Comment */\nvar foo = 1;', | |||
| source: () => '/*! Legal Comment */\nvar foo = 1;', | |||
There was a problem hiding this comment.
Also fix basic test, because /* Comment */ is not legal comment and test1.js.LICENSE is empty, but we expect below what test1.js.LICENSE is not empty
|
Also need refactor tests, but we can do this before next minor/major release, i will send PR in future |
src/index.js
Outdated
| @@ -22,7 +22,7 @@ class UglifyJsPlugin { | |||
| uglifyOptions = {}, | |||
| test = /\.js$/i, | |||
| warningsFilter = () => true, | |||
| extractComments = false, | |||
| extractComments = true, | |||
There was a problem hiding this comment.
This would produce a name.js.LICENSE file by default or just preserves the comments within the file ? Is it really necessary to produce this file by default instead of opt-in if needed ?
semver-ness is also debatable here...
There was a problem hiding this comment.
@michael-ciniawsky yep, it is output name.js.LICENSE file, same as in @0.4.6 version
There was a problem hiding this comment.
Was outputting the file.js.LICENSE the default in v0.4.6 ? I can't remember... 😄 and if so do we really want it to be the default ?
There was a problem hiding this comment.
@michael-ciniawsky ok let's test this 😄 i will put screen shot 👍
There was a problem hiding this comment.
@michael-ciniawsky i was right 😄
There was a problem hiding this comment.
@michael-ciniawsky Good idea
There was a problem hiding this comment.
@michael-ciniawsky from slack
sokra [4:27 PM]
I would keep comments in code by default. LICENSE file by default could be confusing for users and the benefit is small...
There was a problem hiding this comment.
michael-ciniawsky
changed the title
michael-ciniawsky
changed the title
options.extractComments)
#200 (needs a rebase) |
alexander-akait
force-pushed
the
issue-222
branch
from
8b02a59
to
4b5d66d
Compare
|
@michael-ciniawsky friendly ping. Also:
|
alexander-akait
force-pushed
the
issue-222
branch
from
4b5d66d
to
328ba48
Compare
alexander-akait
force-pushed
the
issue-222
branch
from
328ba48
to
efd3e75
Compare
| @@ -42,7 +42,7 @@ class UglifyJsPlugin { | |||
| exclude, | |||
| uglifyOptions: { | |||
| output: { | |||
| comments: false, | |||
| comments: extractComments ? false : /^\**!|@preserve|@license|@cc_on/, | |||
There was a problem hiding this comment.
Shouldn't this be just the {RegExp} to always preserve this comments ? Why is the extractComments check needed here ?
There was a problem hiding this comment.
@michael-ciniawsky When we use extractComments we should remove their from source code. If you pass extractComments: true and comments: /^\**!|@preserve|@license|@cc_on/ output will be contain legal comments in source file and generate name.js.LICENSE file
There was a problem hiding this comment.
Isn't extractComments not only responsible for only extracting what's preseverd via uglifyOptions.output.comments ?
// Preserve comments based on options,
// defaults to /^\**!|@preserve|@license|@cc_on/
if (uglifyOptions.output.comments) {
// Extract all preserved comments
if (options.extractComments) {
// [name].js.LICENSE
extractComments()
}
// Leave comments in the source
}Why is are these options 'decoupled' ? 🙃
Anyways if that's more complicated atm and this PR now leaves the default preserved comments in the source aswell as options.extractComments also preserves the same default comments (with it's own logic) when extracting them into a separate file, then let's do it :)
There was a problem hiding this comment.
@michael-ciniawsky extractComments does not responsible uglify comments, they are separatly. If think about your logic. But sometimes people leave comment in source code and generate license file to allow difference type of distribution.
|
@evilebottnawi In which version of |
|
@evilebottnawi There is nothing left to do here for the moment right ? :) In case there is please chime in, otherwise I will land and release this today... |
|
@michael-ciniawsky let's release 👍 |
michael-ciniawsky
added
status: Approved
and removed
pr_action: Review
priority: 4 (important)
labels
|
Released in |
|
@michael-ciniawsky at npm there is still |
Issues