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.
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
feat: omit html tag attribute with null/undefined/false value #1598
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
}) | ||||||
.map(function (attributeName) { | ||||||
if (tagDefinition.attributes[attributeName] === true) { | ||||||
|
@@ -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 {Object.<string, string | boolean>} [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 {Object.<string, string | boolean>} [meta] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also fixed the JSDoc definitions for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course, do you want me to restore both changes on the JSDoc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes and please also for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The change on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right :) sorry I didn't see
Suggested change
|
||||||
* meta information about the tag e.g. `{ 'plugin': 'html-webpack-plugin' }` | ||||||
* | ||||||
* @returns {HtmlTagObject} | ||||||
*/ | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1198,38 +1198,42 @@ describe('HtmlWebpackPlugin', () => { | |
null, done, false, false); | ||
}); | ||
|
||
it('allows events to remove an attribute by setting it to false', done => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 => { | ||
|
There was a problem hiding this comment.
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:
It has better readability, but due to lint constraints that was not possible.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 excludesfalse
,0
,""
,null
,undefined
andNaN
values.As you can see,
0
as value might be needed.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g.:
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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:This fits nicely
There was a problem hiding this comment.
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:
tagDefinition.attributes[attributeName] === '' || tagDefinition.attributes[attributeName]
which does not allow numbersI am fine with both ways :)
There was a problem hiding this comment.
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