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 create node options when is undefined #1076

Merged
merged 2 commits into from Dec 5, 2021

Conversation

zaceno
Copy link
Contributor

@zaceno zaceno commented Nov 27, 2021

When using custom elements that define an is property, e.g. h('custom-foo', {is: 'button'}), then hyperapp will internally create it like this:

document.createElement('button', {is: 'custom-button'})

But whenever we don't use the is prop (as is generally the case), the call becomes

document.createElement('button', {is: undefined})

Which is not correct as far as I can tell. When there is no is, there should be no second argument at all. This PR fixes the behavior so the call becomes:

document.createElement('button', undefined)

Although browsers seem to handle the current behavior just as well, the current behavior becomes a problem with some custom element libraries that wrap the native document.createElement (don't ask me why... – I guess because of legacy cross-browser concerns?)

For example consider this example of using a-frame. It imports my modified hyperapp, but notice how it breaks when you import the latest published version of hyperapp from cdn.

@jorgebucaran
Copy link
Owner

Do you have any idea what would happen if we just createElement(..., props)? 😏

@zaceno
Copy link
Contributor Author

zaceno commented Nov 28, 2021

Do you have any idea what would happen if we just createElement(..., props)? 😏

Not sure but I assume browsers would handle it ok, but custom-element libraries like https://github.com/dmarcos/document-register-element (which a-frame uses) would break the same way because if there is an object given as the second argument they do not expect props.is to be undefined. And they would be right, at least according to MDN:

An optional ElementCreationOptions object, containing a single property named is, whose value is the tag name of a custom element previously defined via customElements.define().

index.js Outdated
? document.createElementNS(SVG_NS, vdom.tag, { is: props.is })
: document.createElement(vdom.tag, { is: props.is })
? document.createElementNS(SVG_NS, vdom.tag, is && {is})
: document.createElement(vdom.tag, is && {is})
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the late response.

Perhaps props.is && props would be fewer bytes and work just as well. What do you think?

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’ll give it a try!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call – saves 8 bytes (minified and gzipped)! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And works just as well!

Copy link
Owner

@jorgebucaran jorgebucaran left a comment

Choose a reason for hiding this comment

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

I usually prettify the code (via prettier). Do you mind doing that as well?

@zaceno
Copy link
Contributor Author

zaceno commented Dec 5, 2021

I usually prettify the code (via prettier). Do you mind doing that as well?

No problem, I can do that. Would you mind sending your prettierrc (unless you are using their defaults?) I have prettier automatically deactivated in projects that don’t have a prettierrc file in them, because I rules differ and I always end up causing some unintentional diff. But I can apply your rules and just not submit the prettierrc file.

@jorgebucaran
Copy link
Owner

The only rule I have is no semicolons. 😅

@jorgebucaran jorgebucaran added the enhancement New feature or request label Dec 5, 2021
@jorgebucaran jorgebucaran merged commit 3a1318b into jorgebucaran:main Dec 5, 2021
@zaceno
Copy link
Contributor Author

zaceno commented Dec 5, 2021

🎉

skanne pushed a commit to skanne/hyperapp that referenced this pull request Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants