-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
lib/html-tags.js
Outdated
return tagDefinition.attributes[attributeName] !== false; | ||
return tagDefinition.attributes[attributeName] !== false && | ||
tagDefinition.attributes[attributeName] !== null && | ||
tagDefinition.attributes[attributeName] !== undefined; |
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:
[null, undefined, false].includes(tagDefinition.attributes[attributeName])
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 excludes false
, 0
, ""
, null
, undefined
and NaN
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:
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.
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.:
/**
* @param {undefined | null | string | number | boolean} value
*/
function hasValue(value) {
if (value === undefined || value === null || value === false) {
return false;
}
return true;
}
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:
tagDefinition.attributes[attributeName] === '' || tagDefinition.attributes[attributeName]
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:
- The one you already committed (which also allows numbers)
- The
tagDefinition.attributes[attributeName] === '' || tagDefinition.attributes[attributeName]
which does not allow numbers
I 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
lib/html-tags.js
Outdated
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed the JSDoc definitions for object
entry
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.
Can we please keepattributeName
and add undefined
& null
?
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and please also for typings.d.ts
:)
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 change on typingins.d.ts
are necessary to reflect the change
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.
You are right :) sorry I didn't see typings.d.ts
- here it should be the same as in typings.d.ts
:
* @param {Object.<string, string | boolean>} [meta] | |
* @param {{[attributeName: string]: string | boolean | null | undefined}} [meta] |
spec/basic.spec.js
Outdated
@@ -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 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
Sumary
This
pull request
update the html tag parsing to also omit attributes that containsnull
andundefined
value.The motivation behind this change you can find on the Issue #1597.