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: omit html tag attribute with null/undefined/false value #1598

Merged
merged 3 commits into from Feb 12, 2021
Merged

feat: omit html tag attribute with null/undefined/false value #1598

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c33f5c
feat: omit html tag attribute with null/undefined/false value
themgoncalves Feb 12, 2021
7821fb1
refactor: revert changes on JSDoc
themgoncalves Feb 12, 2021
5e753d1
refactor: change implementation per to code review
themgoncalves Feb 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions lib/html-tags.js
View file
Open in desktop
Expand Up @@ -25,12 +25,14 @@ const voidTags = ['area', 'base', 'br', 'col', 'embed', 'hr', 'img', 'input', 'k
* A tag element according to the htmlWebpackPlugin object notation
*
* @param xhtml {boolean}
* Wether the generated html should add closing slashes to be xhtml compliant
* Whether the generated html should add closing slashes to be xhtml compliant
*/
function htmlTagObjectToString (tagDefinition, xhtml) {
const attributes = Object.keys(tagDefinition.attributes || {})
.filter(function (attributeName) {
return tagDefinition.attributes[attributeName] !== false;
return tagDefinition.attributes[attributeName] !== false &&
tagDefinition.attributes[attributeName] !== null &&
tagDefinition.attributes[attributeName] !== undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, this assertion could be rewritten as the following:

[null, undefined, false].includes(tagDefinition.attributes[attributeName])

It has better readability, but due to lint constraints that was not possible.

Copy link
Owner

@jantimon jantimon Feb 12, 2021 •
edited

Choose a reason for hiding this comment

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

What about tagDefinition.attributes[attributeName] === '' || tagDefinition.attributes[attributeName]?

That should be equal to your code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would introduce a bug in the plugin due to the nature of truthy values, which excludes false, 0, "", null, undefined and NaN values.

As you can see, 0 as value might be needed.

Copy link
Owner

Choose a reason for hiding this comment

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

but in your typings you didn't allow numbers - should we add them too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's actually a good point! I'm not sure about the broad use case of it, but if we're going to proceed in this way, then we can simply refactor it to:

  const attributes = Object.keys(tagDefinition.attributes || {})
    .filter(function (attributeName) {
      return tagDefinition.attributes[attributeName];
    })

This would satisfy my use case, although I would appreciate your feedback regarding it, as you might have a better sense of the community usage.

Copy link
Owner

Choose a reason for hiding this comment

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

e.g.:

/**
 * @param {undefined | null | string | number | boolean} value
 */
function hasValue(value) {
  if (value === undefined || value === null || value === false) {
    return false;
 }
 return true;
}

Copy link
Owner

Choose a reason for hiding this comment

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

Or we just keep your initial proposal :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea to have this hasValue is interesting, but if the usage is restricted to this single case, have the assessment inline provides better understand about the code, so I would go for that suggestion of yours:

tagDefinition.attributes[attributeName] === '' || tagDefinition.attributes[attributeName]

This fits nicely

Copy link
Owner

Choose a reason for hiding this comment

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

Okay cool so we have two valid options I guess:

  1. The one you already committed (which also allows numbers)
  2. The tagDefinition.attributes[attributeName] === '' || tagDefinition.attributes[attributeName] which does not allow numbers

I am fine with both ways :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have applied the suggestion you made, as you have a better understanding about its usage.

Thank you for the nice brainstorming

})
.map(function (attributeName) {
if (tagDefinition.attributes[attributeName] === true) {
Expand All @@ -49,13 +51,13 @@ function htmlTagObjectToString (tagDefinition, xhtml) {
* @param {string} tagName
* the name of the tag e.g. 'div'
*
* @param {{[attributeName: string]: string|boolean}} [attributes]
* @param {{[attributeName: string]: string|boolean|null|undefined}} [attributes]
* tag attributes e.g. `{ 'class': 'example', disabled: true }`
*
* @param {string} [innerHTML]
*
* @param {{[attributeName: string]: string|boolean}} [meta]
* meta information about the tag e.g. `{ 'pluhin': 'html-webpack-plugin' }`
* @param {{[attributeName: string]: string|boolean|null|undefined}} [meta]
* meta information about the tag e.g. `{ 'plugin': 'html-webpack-plugin' }`
*
* @returns {HtmlTagObject}
*/
Expand Down
62 changes: 33 additions & 29 deletions spec/basic.spec.js
View file
Open in desktop
Expand Up @@ -1198,38 +1198,42 @@ describe('HtmlWebpackPlugin', () => {
null, done, false, false);
});

it('allows events to remove an attribute by setting it to false', done => {
Copy link
Owner

Choose a reason for hiding this comment

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

please keep the old test and just add 2 more - 1 for null 1 for undefined..

that way we get better error messages if 1 test fails and to guarantee that the old tests are not modified and still work

const examplePlugin = {
apply: function (compiler) {
compiler.hooks.compilation.tap('HtmlWebpackPlugin', compilation => {
HtmlWebpackPlugin.getHooks(compilation).alterAssetTags.tapAsync('HtmlWebpackPluginTest', (pluginArgs, callback) => {
pluginArgs.assetTags.scripts = pluginArgs.assetTags.scripts.map(tag => {
if (tag.tagName === 'script') {
tag.attributes.async = false;
}
return tag;
it('allows events to remove an attribute by setting it to null/undefined/false', done => {
const types = [null, undefined, false];

types.forEach(function (type) {
const examplePlugin = {
apply: function (compiler) {
compiler.hooks.compilation.tap('HtmlWebpackPlugin', compilation => {
HtmlWebpackPlugin.getHooks(compilation).alterAssetTags.tapAsync('HtmlWebpackPluginTest', (pluginArgs, callback) => {
pluginArgs.assetTags.scripts = pluginArgs.assetTags.scripts.map(tag => {
if (tag.tagName === 'script') {
tag.attributes.async = type;
}
return tag;
});
callback(null, pluginArgs);
});
callback(null, pluginArgs);
});
});
}
};
testHtmlPlugin({
mode: 'production',
entry: {
app: path.join(__dirname, 'fixtures/index.js')
},
output: {
path: OUTPUT_DIR,
filename: '[name]_bundle.js'
}
};
testHtmlPlugin({
mode: 'production',
entry: {
app: path.join(__dirname, 'fixtures/index.js')
},
output: {
path: OUTPUT_DIR,
filename: '[name]_bundle.js'
},
plugins: [
new HtmlWebpackPlugin(),
examplePlugin
]
},
plugins: [
new HtmlWebpackPlugin(),
examplePlugin
]
},
[/<script defer="defer" src="app_bundle.js"><\/script>[\s]*<\/head>/],
null, done, false, false);
[/<script defer="defer" src="app_bundle.js"><\/script>[\s]*<\/head>/],
null, done, false, false);
});
});

it('provides the options to the afterEmit event', done => {
Expand Down
2 changes: 1 addition & 1 deletion typings.d.ts
View file
Open in desktop
Expand Up @@ -263,7 +263,7 @@ declare namespace HtmlWebpackPlugin {
* E.g. `{'disabled': true, 'value': 'demo'}`
*/
attributes: {
[attributeName: string]: string | boolean;
[attributeName: string]: string | boolean | null | undefined;
};
/**
* The tag name e.g. `'div'`
Expand Down