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

A better solution for ElementInternals #1036

Closed
trusktr opened this issue Nov 15, 2023 · 2 comments
Closed

A better solution for ElementInternals #1036

trusktr opened this issue Nov 15, 2023 · 2 comments

Comments

@trusktr
Copy link
Contributor

trusktr commented Nov 15, 2023

As I'm starting to use this.attachInternals() in the real world (because only recently did all browsers support the feature), I find that element "internals" are just too difficult to keep internal, especially when trying to use the feature with JavaScript patterns like class-factory mixins that should be composable.

For now I simply settle with public properties for simplicity, but it has problems. Example:

export function WithShadow(Base) {
	return class WithShadow extends Base {
		static shadowMode = "open"

		constructor(...args) {
			super(...args)

			this.internals ??= this.attachInternals()

			if (!this.internals.shadowRoot) this.attachShadow({mode: this.constructor.shadowMode})
		}
	}
}

class SomeEl extends WithShadow(HTMLElement) {
  static shadowMode = 'closed'

  connectedCallback() {
    console.log(this.internals.shadowRoot)
  }
}
customElements.define('some-el', SomeEl)

Will someone decide to mess with element.internals? Who knows. If they break something, oh well.

The problem with this.attachInternals()

This is still not ideal, because it means this mixin is incompatible with other mixins or classes that do not use this.internals as a convention. Any classes that store internals on some other property will cause a runtime error.

A protected feature in JavaScript is nowhere near being in the picture, and may never be at this pace, so that's not something we can simply tell people to wait for. And even if it existed, the same issue exists: not all classes will be standardized to use the same property, so the runtime error can still happen when using the same pattern with a protected field.

The main problem right now is that the pattern I choose for making internals shared across classes can conflict with some other author's existing pattern, making classes/libraries incompatible, and a protected feature for JavaScript cannot fix that.

Incompatibility example:

import {WithShadow} from 'above'
import {WithStates} from 'below'

class MyEl extends WithShadow(WithStates(states, HTMLElement)) {
  shadowMode = 'closed'
  states = { /* ... imagine some declarative way to define states based on conditions, or something ... */ }

  connectedCallback() {
    console.log(this.internals.shadowRoot, ...this.internals.states.keys())
  }
}
customElements.define('my-el', MyEl)

new MyEl() // <--------- Runtime error because `attachInternals` gets mistakenly called twice.
// below
export function WithStates(Base) {
	return class WithStates extends Base {
		constructor(...args) {
			super(...args)

			// using a different property name here
			this.__internals ??= this.attachInternals()

			// ... this.__internals.states is manipulated based on the `this.states` definition, or something ...
		}
	}
}

This is highly non-composable without strong conventions that all custom element libraries would not be guaranteed to follow.

A better solution today?

EDIT: I realized that with new SomeCustomElement there's no way for the browser to call an internals callback after user constructor, so the current section's idea will not work. Further below, the class decorator idea can work out.

I'm starting to think that a standardized callback for internals would be a better way to expose internals and keep them encapsulated, as described to some extent in

It works better because then there's a standard named member that all custom element classes can rely on with which they can receive internals without leaking to public.

With a callback, there is no conflict of property naming, each class can use a #private field without issue, like so:

class CoolElement extends HTMLElement {
  #internals
  
  internalsCallback(internals) {
    this.#internals = internals
    this.#doSomethingWithInternals()
  }
  
  #doSomethingWithInternals() { console.log(this.#internals) }
}

class CoolElementWithMoreFeatures extends HTMLElement {
  #stuff
  
  internalsCallback(internals) {
    super.internalsCallback(internals)

    this.#stuff = internals
    this.#doSomethingWithStuff()
  }
  
  #doSomethingWithStuff() { console.log(this.#stuff) }
}

class CoolElementWithEvenMoreFeatures extends HTMLElement {
  #things
  
  internalsCallback(internals) {
    super.internalsCallback(internals)

    this.#things = internals
    this.#doSomethingWithThings()
  }
  
  #doSomethingWithThings() { console.log(this.#things) }
}

const el = new CoolElementWithEvenMoreFeatures()

el.#internals // error
el.#stuff // error
el.#things // error

// either this is allowed:
el.internalsCallback(new MyMockInternals())

// or maybe the browser enforces a special invariant that the method cannot be called after construction:
el.internalsCallback(something) // runtime error

A better solution in the decorated future?

Extending the previous idea to enforce the invariant

How can that invariant idea be enforced? It would be easy to enforce with document.createElement() because it can easily run logic after construction to delete the internalsCallback method. But with usage of new, the engine would need a special non-standard way to run logic after construction to ensure that the method is deleted after new MyElement is finished, or some other special non-standard JS behavior (not sure if that's possible across browsers).

When decorators reach stage 4 (looks like they have a very good chance of doing so now, but browser implementations are going really slow in this area) then another idea for enforcing the invariant is to require usage of a decorator, because decorators can always add logic that runs after the user's contructor.

Example:

@customElements.withInternals
class MyEl extends HTMLElement {
  internalsCallback(internals) { ... }
}

where customElements.withInternals is a decorator provided natively by the browser's customElements API, and where internalsCallback will be called during construction only if the decorator is used, and where the decorator has the final say in finishing the construction so it can delete internalsCallback (and if for some reason it cannot be deleted, throws an error).

With new.target, the decorator can ensure that in a hiearchy of such decorated classes, only the outermost class performs the finalization:

@customElements.withInternals
class MyEl extends HTMLElement {
  internalsCallback(internals) { ... }
}

@customElements.withInternals
class BetterEl extends HTMLElement {
  internalsCallback(internals) { ... }
}

new BetterEl() // internalsCallback is deleted only after the `BetterEl` constructor, not after `MyEl`.

Could such an idea be viable?

Alternatively with a method decorator

Another idea is maybe there's a way to do it with a built-in decorator that injects internals directly to a method:

class MyEl extends HTMLElement {
  @customElements.withInternals
  myArbitrarilyNamedInternalsHandler(internals) { ... }
}

TLDR

These solution ideas keep the internals actually protected (just not in the form of a protected field which would still have the naming problem). With these ideas, a user of an element cannot get the internals from the outside (although they can try to mock it in the case without the invariant, and element authors can guard against this if they wish), while all classes that all custom element authors could possibly write will have a standard way of sharing protected internals without conflicting patterns.

It is possible to add these new solutions later, while leaving this.attachInternals so that it would still work (if not deprecating it and documenting so in places like MDN (and maybe even eventually removing it)).

With the new solution in place, custom element authors begin to stop using this.attachInternals (new elements would be designed with the new API up front, and existing libraries would start to convert to the new solution over time). Precendence for this exists with Custom Elements v0, and DOM Mutation Events which are deprecated although all browsers still have the API.

@trusktr
Copy link
Contributor Author

trusktr commented Nov 16, 2023

Oops, I realized that with new SomeElement there's actually no way for the browser to call an internals callback like it could with document.createElement('some-element') or during parsing, unless we wait for decorators.

I'm guessing this is why the internals callback idea was not viable (decorators were not even stage 3 at the time).

Only a class decorator can ensure that the internals callback is called during construction with new after the user constructor so that private fields are ready.

The method decorator idea won't work: it can add an initializer in which could call the internals callback during the user's constructor but the initializer runs before fields are initialized, which means users will not be able to use private fields in the callback because they'll get a runtime error:

function methodDeco(value: unknown, context: ClassMethodDecoratorContext) {
  context.addInitializer(function(this: any) {
    this[context.name]({foo: 123})
  })
}

class MyClass {
  #foo: any

  @methodDeco receiveInternals(obj: {foo: number}) {
    this.#foo = obj
  }

  logInternals() {console.log(this.#foo)}
}

new MyClass().logInternals() // TypeError: Cannot write private member to an object whose class did not declare it

TS playground (hit "Run" to see the error)

A class decorator would work like so:

function classDeco<T extends new (...args: any[]) => any>(value: T, context: ClassDecoratorContext) {
  return class extends value {
    constructor(...args: any[]) {
      super(...args)
      this.internalsCallback({foo: 123})
    }
  }
}

@classDeco
class MyClass {
  #foo: any

  internalsCallback(obj: {foo: number}) {
    this.#foo = obj
  }

  logInternals() {console.log(this.#foo)}
}

new MyClass().logInternals()

TS playground (click "Run" and see console output)

@trusktr
Copy link
Contributor Author

trusktr commented Nov 16, 2023

Closing, I'll re-open a better one with wrong parts removed.

@trusktr trusktr closed this as completed Nov 16, 2023
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

No branches or pull requests

1 participant