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

Initial implementation of hot elements. #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rictic
Copy link
Contributor

@rictic rictic commented May 26, 2020

This just provides the interface that overrides customElements.define. My thinking is that the LitElement implementation of notifyOnHotModuleReload would live (and be tested) in the lit-element repo, as it needs to access private internals of LitElement.

@rictic rictic requested a review from justinfagnani May 26, 2020 15:10
Copy link

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

LGTM. Just some passing thoughts...

(maybe: Constructor<HTMLElement>): maybe is HotReloadableElementClass => {
// This isn't safe against name mangling, but this is definitely debug
// code, so that's fine.
return 'notifyOnHotModuleReload' in maybe;

Choose a reason for hiding this comment

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

how about using typeof maybe.notifyOnHotModuleReload === 'function' for the duck typing? That's how Thenables work, afaik.

const esmHmrIsPresent = !!(import.meta as HotImportMeta).hot;
/**
* This code shouldn't be imported in prod, but if it is, try to give any
* bundler as many chances as it can to dead code eliminate it.

Choose a reason for hiding this comment

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

From what I've seen, the DEC is often weak and I'm not sure it'll always propagate out of statements like this. You might have a better chance if this is in the if clause below. Over time it'd be good to document here that this was tested in which tools.

const implMap = new Map<string, HotReloadableElementClass>();
const hotDefine: typeof customElements.define = function hotDefine(
tagname, classObj) {
if (!isHotReloadableElementClass(classObj)) {

Choose a reason for hiding this comment

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

You may want to consider eventually adding some support for elements without this callback - maybe proxy the class so that new instances are created with the new class, or so the prototype is swapped out. This could be an opt-in workaround against some environments that try to do hot reloads by default, and maybe it'd do something kind of useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, figuring out how best to leverage proxies here is definitely on the list of things to do here

* @param updatedClass The newer version of this class
* (or some other class that's trying to claim this class' tagname).
*/
notifyOnHotModuleReload(

Choose a reason for hiding this comment

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

I would consider naming this with a Callback suffix like the lifecycle callbacks.

@@ -0,0 +1,28 @@
BSD 3-Clause License

Copyright (c) 2019, The Polymer Authors. All rights reserved.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Copyright (c) 2019, The Polymer Authors. All rights reserved.
Copyright (c) 2020, The Polymer Authors. All rights reserved.

@douglascvas
Copy link

Do you guys plan to merge this any soon?

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