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

refactor(engine): remove dependency on Element.prototype.tagName #1889

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

pmdartus
Copy link
Member

Details

This PR removes the dependency on Element.prototype.tagName to look up the component name. The tag name is now stored directly on the VM.

Does this PR introduce breaking changes?

  • No, it does not introduce breaking changes.

If yes, please describe the impact and migration path for existing applications.

The PR fulfills these requirements:

  • Have tests for the proposed changes been added? ✅
  • Have you followed these instructions to clearly describe the issue being fixed or feature enhanced? ✅

GUS work item

W-7552459

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

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

:shipit:

@ekashida ekashida merged commit bafb31c into master Jun 2, 2020
@ekashida ekashida deleted the pmdartus/tag-name-removal branch June 2, 2020 00:25
@@ -116,6 +116,7 @@ export function createElement(
setElementProto(element, def);

createVM(element, def, {
tagName: sel,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concern that if someone is doing createElement('x-Foo', { is: Foo }), it will ended being an invalid tagName here. For WC we extract it from the element and lowercase it; when from the compiled template, it is probably safer, and always coming as lowercase, but our own createElement do not lowercase it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline and agreed to leave this as-is for now since we couldn't think of a scenario where it would make a difference.

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.

4 participants