Skip to content

Conversation

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Dec 4, 2017

Notable Changes

Add an include && exclude option for assets

Issues

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

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

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... :)

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
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 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
@alexander-akait alexander-akait force-pushed the feature-include-exclude branch from acff2b7 to 6269e9d Compare December 5, 2017 10:54
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 :)

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... :)

if (this.deleteOriginalAssets) {
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 alexander-akait force-pushed the feature-include-exclude branch from 6269e9d to f9feda1 Compare December 6, 2017 12:42
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add option to exclude assets (options.exclude)

3 participants