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

On Demand Definitions Support #31

Open
dgp1130 opened this issue Feb 17, 2025 · 2 comments
Open

On Demand Definitions Support #31

dgp1130 opened this issue Feb 17, 2025 · 2 comments
Labels
enhancement New feature or request

Comments

@dgp1130
Copy link

dgp1130 commented Feb 17, 2025

Context

The On-Demand Definitions Web Components Community Protocol

Problem

<is-land import="..."> effectively requires the imported module to define any relevant custom elements in the top-level scope of the provided module. This requires developers to follow a specific convention:

export class MyComponent extends HTMLElement {
  // ...
}

// Top-level side effect
customElements.define('my-component', MyComponent);

This top-level side effect has a few negative consequences:

  1. It prevents the component from being tree shaken when bundled (admittedly I don't think bundlers work too well <is-land import="..."> anyways).
  2. The imported module may not directly define all the components included underneath an <is-land>, and instead may implicitly define some of them as transitive dependencies. If those transitive dependencies are ever changed, the import may fail to define all the components originally expected by the developer and lead to unintended bugs.

An example of problem 2.:

<is-land import="./my-foo.js">
  <my-foo>
    <my-bar></my-bar>
  </my-foo>
</is-land>
// my-foo.js

import './my-bar.js'; // Import `my-bar` to ensure it is defined.

export class MyFoo extends HTMLElement {
  connectedCallback() {
    // Use `my-bar` if present.
    const bar = this.querySelector('my-bar');
    if (bar) bar.doSomething();
  }
}

customElements.define('my-foo', MyFoo);
// my-bar.js

export class MyBar extends HTMLElement {
  doSomething() { /* ... */ }
}

customElements.define('my-bar', MyBar);

<is-land> only imports ./my-foo.js, but takes advantage of that to define both <my-foo> and <my-bar>. If my-foo.js were refactored to no longer depend on <my-bar>, it might look like:

// my-foo.js

// No longer a need to import `<my-bar>`.
// import './my-bar.js';

export class MyFoo extends HTMLElement {
  // ...
}

customElements.define('my-foo', MyFoo);

The import="./my-foo.js" now no longer defines <my-bar>, therefore the rendered element is a no-op and has no functionality.

Proposal

Use On-Demand Definitions to implicitly define any components which are referenced by import. Consider a new define attribute which references exported symbols from the import attribute and calls .define on them.

A pseudo-code implementation might look like:

export class IsLand extends HTMLElement {
  async hydrate() {
    const mod = await import(this.getAttribute('import'));

    const defineAttr = this.getAttribute('define');
    if (!defineAttr) return;

    const clazz = mod[defineAttr];
    if (!clazz) throw new Error(`Did not export ${defineAttr}`);

    if (!('define' in clazz)) throw new Error('Class does not implement On-Demand Definitions.');
    clazz.define();
  }
}

Then consumers would use it like so:

<is-land import="./my-foo.js" define="MyFoo">
  <my-foo></my-foo>
</is-land>

This would work for any component which implements On-Demand Definitions, meaning MyFoo would need to look something like:

export class MyFoo extends HTMLElement {
  static define() {
    // Check if the tag name was already defined by another class.
    const existing = customElements.get('my-foo');
    if (existing) {
      if (existing === MyFoo) {
        return; // Already defined as the correct class, no-op.
      } else {
        throw new Error(`Tag name \`my-element\` already defined as \`${existing.name}\`.`);
      }
    }

    customElements.define('my-foo', MyFoo);
  }
}

This provides a more direct association between an <is-land> instance and the component classes it depends upon. This requires the defined class to be exported from the given module, meaning it is not a random implementation detail of top-level side-effects triggered by importing that module. And if the class is ever moved or renamed, users will get a clear error message about their mistake.

Other Considerations

Bundlers

Using this feature allows developers to write their code without top-level side-effects in all their components, which makes their code generally friendlier to bundlers and easier to work with (see the problems listed here.) The motivating example provided above is admittedly an edge case, however if there ever becomes a good story for using <is-land> with bundlers (maybe one already exists I'm unaware of), then this feature becomes even more valuable as it would allow tree-shaking unused components in a much more natural fashion.

Multiple Components

As my motivating example uses, it is possible for an <is-land> to load multiple components, therefore it probably makes sense to support multiple component names in a define attribute. I think separating them by a space seems reasonable, given that spaces cannot appear in JavaScript identifiers, so one could write:

<!-- Defines both `MyFoo` **and** `MyBar`. -->
<is-land import="./my-components.js" define="MyFoo MyBar">
  <my-foo></my-foo>
  <my-bar></my-bar>
</is-land>

This approach does require that both symbols are exported from the same module, which might not necessarily be a safe assumption and require developers to produce an otherwise unnecessary my-components.js file solely for re-exporting these two components. However I think this is somewhat of an existing issue with import and applies even without define.

An alternative syntax might be something like:

<is-land import="./my-foo.js#MyFoo ./my-bar#MyBar">
  <!-- ... -->
</is-land>

I'm not sure if a space is entirely appropriate there given that URLs can have spaces (escaped to %20) and that # does have existing meaning in URLs which may or not be desirable in this context. However this approach can support importing specific symbols from distinct modules and avoid the need for a re-export module. Just one consideration.

Since I happen to be particularly invested in finding use cases for On-Demand Definitions and pushing that proposal forward, I'd be happy to put together a PR adding support to <is-land>. Alternatively if you have any particular concerns with the design of the proposal, I'd also be interested in hearing any feedback or suggestions on how to improve it and how applicable it is to <is-land> in particular.

@zachleat
Copy link
Member

zachleat commented Feb 18, 2025

This is great — thank you for writing it up! I have no arguments with the content presented — it all seems sound to me.

Works now: Initialization types

Just thinking aloud, this is possible in is-land v5 with the new initialization types API (you can change the default behavior by assigning a default init type):

Island.addInitType("default", async (target) => {
	const defineAttr = target.getAttribute('define');
	if (!defineAttr) return;

	const mod = await import(target.getAttribute("import");
	const clazz = mod[defineAttr];
	if (!clazz) throw new Error(`Did not export ${defineAttr}`);

	if (!('define' in clazz)) throw new Error('Class does not implement On-Demand Definitions.');
	clazz.define();
});

For non-default types, you would need to associate it with the island with the type attr: <is-land type="my-type-name">.

However, default is a special key for a fallback default initialization type for islands that used an import attr (e.g. <is-land import>) without explicitly having a type assigned.

} else if(this.getAttribute(Island.attr.import)) {

Currently there is no way to assign multiple initialization types to a specific island.

Works now: bundler friendly

For the bundler specific question, it might be beneficial to have more a static analysis friendly dynamic import, but that means a init type will be limited to one specific element, e.g. <is-land type="define:my-foo">:

Island.addInitType("define:my-foo", async (target) => {
	const defineAttr = target.getAttribute('define');
	if (!defineAttr) return;

	const mod = await import("./my-foo.js");
	const clazz = mod[defineAttr];
	if (!clazz) throw new Error(`Did not export ${defineAttr}`);

	if (!('define' in clazz)) throw new Error('Class does not implement On-Demand Definitions.');
	clazz.define();
});

I don’t really like the above as it’s way too verbose.

Idea: support multiples

What if we added support for multiple types that run sequentially? e.g. <is-land type="my-foo define">:

// This doesn’t currently work
Island.addInitType("my-foo", async (target) => {
	// return object reassigns `mod` param for next type
	return import("./my-foo.js");
});
Island.addInitType("define", (target, mod) => {
	const defineAttr = target.getAttribute('define');
	if (!defineAttr) return;

	const clazz = mod[defineAttr];
	if (!clazz) throw new Error(`Did not export ${defineAttr}`);

	if (!('define' in clazz)) throw new Error('Class does not implement On-Demand Definitions.');
	clazz.define();
});

How do you feel about these options? Do they feel better than diving deeper into more complex import attr syntax?

@dgp1130
Copy link
Author

dgp1130 commented Feb 19, 2025

Thanks @zachleat! Island.addInitType is interesting as a mechanism to be more compatible with bundlers and to add this behavior from userspace.

As you mention though, it is a bit verbose, and asking every usage of Island.addInitType to duplicate the on-demand definition code feels overly burdensome.

1. Implicit definition in addInitType

How would you feel about automatically triggering the on-demand definition on the returned value? In this way the following would be implicitly defined.

// my-foo.js

export class MyFoo extends HTMLElement {
  static define() { /* ... */ }
}
// usage.js

Island.addInitType('my-foo', () => import('./my-foo.js').then((m) => m.MyFoo));

I guess in this scenario Island.addInitType would need to no-op if the return value is not a custom element class, but then call .define if it is a custom element class. Based on the examples in the README, it seems like the expected usage of this function is to side-effectfully initialize the loaded content, which doesn't quite align here.

2. Manual define

We could lean into the side-effectful nature of Island.addInitType and just call define directly:

Island.addInitType('my-foo', async () => {
  const {MyFoo} = await import('./my-foo.js');
  MyFoo.define();
});

I don't hate this as it would be fully type-checkable and doesn't need to rely on <is-land> to validate the result like direct implementation in import would. The downside here is that developers need to remember to call .define manually which doesn't quite align with the general goals of the proposal. Ideally this is something developers wouldn't even need to think about because <is-land> would just define the element when needed.

3. New Primitive

Based on the fact that Island.addInitType expects a side-effect to initialize the element, maybe a different primitive would be appropriate to "return a custom element class for Island to define".

Island.addDefinition('my-foo', () => import('./my-foo.js').then((m) => m.MyFoo));

Here, I'm imagining an Island.addDefinition function (name subject to bikeshedding) which exists to associate a specific tag name to a loader which will define that element. This could be implemented as simply as:

interface Defineable {
  new (): HTMLElement;
  define(): void;
}

export class Island extends HTMLElement {
  private readonly definitionMap = new Map<string, () => Promise<Defineable>();

  addDefinition(tagName: string, loader: () => Promise<Defineable>) {
    this.definitionMap.set(tagName, loader);
  }

  async hydrate(): Promise<void> {
    const el = // ... - Element to hydrate.

    const definition = this.definitionMap.get(el.tagName.toLowerCase());
    if (!definition) throw new Error('...'); // Probably want to fallback to existing behavior?

    const clazz = await definition();
    if (clazz ) {
      clazz?.define();
      if (!(el instanceof clazz)) throw new Error('...');
    }
  }
}

This would allow you to set definitions for any custom elements, and Island would automatically load them when discovered.

A side benefit is that you can use this to dynamically load multiple elements inside a single <is-land> and it would appropriately load all of them. You also don't need to manually set type, because the tag name is all you need. Downside is that Island.addDefinition might be called multiple times for the same tag if it's used in multiple <is-land> elements and not deduplicated. You'd probably need to no-op when called subsequently on an existing tag, but there's no real way to guarantee that the provided definition function is equivalent.

A variant of this could allow side-effectful definitions in the Island.addDefinition callback, if you'd rather go that direction.

4. Higher-Level of Abstraction

The one other approach I can think of is to basically accept manual define as the solution for <is-land> and then make it easy to generate that pattern at a higher-level of abstraction (read: Eleventy).

I've been experimenting with something similar in @rules_prerender, where you can (at build-time):

  1. Associate a tag name with a custom element definition
  2. Render that tag.
  3. The framework then automatically codegens a script to call .define on the referenced custom element class

(Bonus points, skip load / .define() call when defer-hydration is set.)

We could do something similar with any frameworks wrapping <is-land>, such as Eleventy. The exact approach doesn't necessarily need to match what @rules_prerender does, but the point is to make it easy to go from "render a custom element" to "codegen Island.addInitType and link the dynamic import() in a bundle-able fashion."

This is basically my thinking for how to handle "entry point components" and it seems relevant here, but I'm still just in the experimenting phase.

I guess the key question here is whether <is-land> is at the right level of abstraction to use on-demand definitions, and if so, whether addInitType or import is the right primitive to build upon or whether a new primitive would make the most sense. My personal intuition is that we shouldn't be asking users to write .define themselves, people just won't remember to do it. Therefore it's best to have it be naturally called as a consequence of loading a component. That eliminates 2., but I don't have strong feelings between the others.

I do think it would be cool for import to support this kind of feature, but I also recognize that the main value comes from having tree-shakable components with bundlers, so realistically, supporting import is likely to have noticeably less impact than supporting something at the JS layer (Island.addInitType), so maybe it's best to start there at least.

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

No branches or pull requests

2 participants