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.

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

[fix] null value for base tag prevents render #192

Merged
merged 3 commits into from
Nov 16, 2016

Conversation

cwelch5
Copy link
Contributor

@cwelch5 cwelch5 commented Nov 6, 2016

Continuation of null values fix #165 - this commit fixes the same issue for the base tag (as it uses a different function to calculate tag)

Unit tests updated to cover undefined/null values for the whole API.

…ndefined/null values for primary attributes of a tag) - this commit fixes the same issue for the base tag (as it uses a different function to calculate tag)

Unit tests updated to cover undefined/null values for the whole API.
<Helmet
script={[
{
src: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this impact innerHTML support for script? Can you add a test for when src is undefined but innerHTML is defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To your point, yes, this would not render a tag:

{
    innerHTML: "function test() {console.log('test');} test();",
    src: undefined
}

because it found a primary key without a value, BUT you should really never be specifying more than one primary key for your tag.

This, on the other hand, will render a tag because the for loop only uses one primary key, and takes the last one it finds:

{
    src: undefined,
    innerHTML: "function test() {console.log('test');} test();"
}

But again, this use case would be misuse.

@cwelch5 cwelch5 merged commit 5df0bb2 into master Nov 16, 2016
@cwelch5 cwelch5 deleted the feature/null-values-follow-up branch December 4, 2016 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants