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.

Sign up for GitHub

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

Jump to bottom

feat: add include and exclude options (options.include|options.exclude) #82

Merged
merged 1 commit into from
Dec 7, 2017
Merged

feat: add include and exclude options (options.include|options.exclude) #82

merged 1 commit into from
Dec 7, 2017

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Dec 4, 2017 •
edited by michael-ciniawsky

Notable Changes

Add an include && exclude option for assets

Issues

Also fix eslint problems and fix typo('`') in documentation

michael-ciniawsky
michael-ciniawsky approved these changes Dec 4, 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.

👍 Very nice

src/index.js Outdated
@@ -14,6 +15,7 @@ class CompressionPlugin {
this.compressionOptions = {};

if (typeof this.algorithm === 'string') {
// eslint-disable-next-line global-require
Copy link
Member

Choose a reason for hiding this comment

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

Please always move eslint-disable stuff to the top of the particular file :)

/* eslint-disable
* global-require,
* more rules...,
*/

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 i think it is bad practice because disable rule on all file can provoke errors in places where I would not want to ignore them. Disabled rule should be on line (before line) where i want disable, not on all file.

Copy link
Member

Choose a reason for hiding this comment

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

@evilebottnawi You can also leave them it's not a critical issue by any means, but I would still argue that while you are right for things like eval no-eval should occur where the code is, styling related stuff should be at the top (if needed) to lessen the noise while reading the code. These comments shouldn't be necessary at all normally... :)

src/index.js
@@ -39,15 +41,13 @@ class CompressionPlugin {

apply(compiler) {
compiler.plugin('emit', (compilation, callback) => {
const assets = compilation.assets;
const { assets } = compilation;
// eslint-disable-next-line consistent-return
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment :)

src/index.js Outdated
@@ -82,7 +82,7 @@ class CompressionPlugin {
if (this.deleteOriginalAssets) {
delete assets[file];
}
cb();
return cb();
Copy link
Member

Choose a reason for hiding this comment

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

cb();

return null;

?

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 kk

@michael-ciniawsky michael-ciniawsky added this to the 1.1.0 milestone Dec 4, 2017
@michael-ciniawsky michael-ciniawsky changed the title feat: exclude and include options feat: add include and exclude options (options.include|options.exclude) Dec 4, 2017
michael-ciniawsky
michael-ciniawsky approved these changes Dec 5, 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.

If nobody objects within the next 48 hours I will merge this PR or someone else merges it in the meantime :)

src/index.js Outdated
@@ -14,6 +15,7 @@ class CompressionPlugin {
this.compressionOptions = {};

if (typeof this.algorithm === 'string') {
// eslint-disable-next-line global-require
Copy link
Member

Choose a reason for hiding this comment

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

@evilebottnawi You can also leave them it's not a critical issue by any means, but I would still argue that while you are right for things like eval no-eval should occur where the code is, styling related stuff should be at the top (if needed) to lessen the noise while reading the code. These comments shouldn't be necessary at all normally... :)

src/index.js Outdated
@@ -83,6 +83,7 @@ class CompressionPlugin {
delete assets[file];
}
cb();
Copy link
Member

Choose a reason for hiding this comment

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

\ncb()\n

@alexander-akait
Copy link
Member Author

@michael-ciniawsky also add newlines before/after if for improve reading

@michael-ciniawsky michael-ciniawsky merged commit 1ce3024 into master Dec 7, 2017
@michael-ciniawsky michael-ciniawsky deleted the feature-include-exclude branch December 7, 2017 23:27
@michael-ciniawsky michael-ciniawsky removed this from the 1.1.0 milestone Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@joshwiens joshwiens Awaiting requested review from joshwiens

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

Successfully merging this pull request may close these issues.

[Feature] Add option to exclude assets (options.exclude)
2 participants
@alexander-akait @michael-ciniawsky