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

Use a single definition per tag name #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rictic
Copy link

@rictic rictic commented Sep 15, 2022

Update it when the tag is redefined.

This way LatestCtor and the callbacks are updated for all instances immediately on redefinition.

I'm not certain this change is an improvement in all cases, but it's the behavior that I think we generally want when doing HMR in google, but I could see the value in making it an option.

…when imported.

The main index file behaves the same so this is backwards compatible.

I'm working on using this library at google, and I want to make sure we only use it in development (using a technique similar to `if (process.env === 'dev') { activate() }`). We also patch HTMLElement in the custom elements es5 shim, and we need to apply that patch before we apply this one, and exporting the activate function makes it somewhat easier to ensure that the patches are applied in the correct order.
…cted with new

We need to use the constructor call trick in the PivotCtor when the element has been created by the browser directly (e.g. document.createElement, or via the parser) because in that case the only constructor that's called is the PivotCtor.

However, if the element is called via `new MyElement()` then the MyElement constructor (and any superclass constructors) will be called properly without any work on our part.

This change fixes a bug we noticed where calling `new MyElement()` was running constructors twice.
Update it when the tag is redefined.

This way LatestCtor and the callbacks are updated for all instances immediately on redefinition.

I'm not certain this change is an improvement in all cases, but it's the behavior that I think we generally want when doing HMR in google, but I could see the value in making it an option.
@caridy
Copy link
Owner

caridy commented Nov 30, 2022

@rictic thanks for the PR, but it is impossible to review... can you update the PR with the actual changes? The diff, even when ignoring the whitespaces, it is the entire file.

@rictic
Copy link
Author

rictic commented Nov 30, 2022

This builds on top of #3 to make it easier to merge them both, if you look at just the second and third commits the change is clearer

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.

None yet

2 participants