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

[element-internals] How to get internals in base class and subclass, without leaking it to public #962

Closed
trusktr opened this issue Jul 26, 2022 · 17 comments

Comments

@trusktr
Copy link
Contributor

trusktr commented Jul 26, 2022

How does a sub class get the internals? This isn't working:

class Base extends HTMLElement {
  #_
  
  constructor() {
    super()
    this.#_ = this.attachInternals()
  }
}

class MyEl extends Base {
  #_
  
  constructor() {
    super()
    this.#_ = this.attachInternals() // subclass also wants it, but this causes runtime error
  }
}

customElements.define('my-el', MyEl)

document.body.append(document.createElement('my-el')) // ERROR

The error in Chrome is:

Uncaught DOMException: Failed to execute 'attachInternals' on 'HTMLElement': ElementInternals for the specified element was already attached.

The Tc39 ES group thought protected was not a worthy thing to give us. What do we do, how do we keep it within the class hierarchy without leaking to public? Implement a complicated dance?

@trusktr
Copy link
Contributor Author

trusktr commented Jul 26, 2022

Maybe, as long as we're still in construction, attachInternals should return existing internals rather than throwing, effectively making it protected-like? Should it perhaps be called getInternals in that case, instead?

@trusktr trusktr changed the title [element-internals] [element-internals] How to get internals in base class and subclass, without leaking it to public Jul 26, 2022
@rniwa
Copy link
Collaborator

rniwa commented Jul 26, 2022

We need an JavaScript equivalent of "protected" keyword in C++/Java to do this.

@keithamus
Copy link
Collaborator

The work-arounds for this, for now, are to either override attachInternals:

class Base extends HTMLElement {
  #internals
  #internalsCalled = false

  constructor() {
    super()
    this.attachInternals()
    this.#internalsCalled = false
  }

  attachInternals() {
    if (this.#internals && !this.#internalsCalled) {
      this.#internalsCalled = true
      return this.#internals
    }
    this.#internals = super.attachInternals()
    this.#internalsCalled = true
    return this.#internals
  }

}

Or to provide some agreed upon interface in your own libraries, like getInternals(), which breaks the privacy contract around attachInternals unless you're able to use TypeScript's protected property in all code.

@trusktr
Copy link
Contributor Author

trusktr commented Jul 28, 2022

@keithamus But in your example, if a subclass of the base class doesn't need the internals (not all end subclasses may need them) then your example leaves the extra call available for the public.

The following is still a problem, isn't it?

customElements.define('my-el', class extends HTMLElement {})
document.createElement('my-el').attachInternals() // returns "internals" to the public. (I had expected an error)

I originally thought that attachInternals could only be called in constructor. It would be enforceable with documet.createElement, but not with new unless the DOM engine can do something special with the JS engine that otherwise normal JS users can't.

And, if attachInternals would be callable only in constructor (otherwise throw), then we could as well rename it to getInternals and allow it to be called any number of times in constructor.

So, if we can't actually guarantee that attachInternals is internal (unless explicitly leaked by an author), why call it attachInternals?

Maybe it should be called attachFeatures or getFeatures because it is clearly public, just throws on any call other than the first (this does not at all guarantee privacy or "internal"ness).

@trusktr
Copy link
Contributor Author

trusktr commented Jul 28, 2022

unless you're able to use TypeScript's protected property in all code.

I'm interested in plain JS semantics because as a library author, I can't guarantee my users will use TypeScript.

@keithamus
Copy link
Collaborator

keithamus commented Jul 28, 2022

@keithamus But in your example, if a subclass of the base class doesn't need the internals (not all end subclasses may need them) then your example leaves the extra call available for the public.

Right. My example just works around the error condition of calling attachInternals twice while preserving it for later callsites. It doesn't attempt to fix the issue of outside access.

If you know you don't want others to have access to internals and you don't want to call attachInternals yourself, you can disable it using disabledFeatures which means it won't be available for public (or just call it in your constructor anyway).

customElements.define('my-el', class extends HTMLElement {
static get disabledFeatures() { return ['internals']; }
})
document.createElement('my-el').attachInternals() // throws

@calebdwilliams
Copy link

You can always save it in a WeakMap and provide some API behind a closure to keep it hidden. It’s not particularly performant but it’s probably the best bet.

Regardless, this is a perfect example of the need for protected class fields in JavaScript.

@bakkot
Copy link

bakkot commented Jul 29, 2022

Is there something wrong with the obvious approach of explicit communication?

class Base extends HTMLElement {
  #_
  
  constructor(forwardInternals) {
    super()
    let internals = this.attachInternals()
    this.#_ = internals
    forwardInternals?.(internals)
  }
}

class MyEl extends Base {
  #_
  
  constructor() {
    // if you want to be open for further extension, repeat the `forwardInternals` thing in this constructor
    let internals;
    super(int => { internals = int })
    this.#_ = internals
  }
}

@trusktr
Copy link
Contributor Author

trusktr commented Jul 29, 2022

customElements.define('my-el', class extends HTMLElement {
static get disabledFeatures() { return ['internals']; }
})
document.createElement('my-el').attachInternals() // throws

That API seems backwards. Perhaps it should have been this:

customElements.define('my-el', class extends HTMLElement {})
document.createElement('my-el').attachInternals() // throws
customElements.define('my-el', class extends HTMLElement {
  static enabledFeatures = ['internals']
})
document.createElement('my-el').attachInternals() // does not throw

Always avoid double negatives!

But that's tangential anyway. Once the feature is available, protecting it is difficult.

@trusktr
Copy link
Contributor Author

trusktr commented Jul 29, 2022

@trusktr
Copy link
Contributor Author

trusktr commented Jul 29, 2022

@bakkot interesting trick. That does indeed keep the internals protected. The only downside is subclasses needing to remember that boilerplate. That subclass constructor should also wire up a callback in case a further subclass needs internals (lots of boilerplate).

And then it gets tricky with class-factory mixins. protected would be nice!

@calebdwilliams
Copy link

With bakkot’s trick, that does cause complications calling new Whatever though. Might not be a deal breaker though.

@bakkot
Copy link

bakkot commented Jul 29, 2022

@trusktr

The only downside is subclasses needing to remember that boilerplate.

Well, if you forget it, you won't get access to the internals, so I don't think there's much risk of that. And if you don't need the internals you don't need to include the boilerplate in your subclass.


@calebdwilliams

that does cause complications calling new Whatever though.

What complications do you mean?

@trusktr
Copy link
Contributor Author

trusktr commented Jul 31, 2022

Well, if you forget it, you won't get access to the internals, so I don't think there's much risk of that. And if you don't need the internals you don't need to include the boilerplate in your subclass.

But if you forget it, then a further subclass of your class can't get them either, so you need the boilerplate even if your class doesn't need internals.

@bakkot
Copy link

bakkot commented Jul 31, 2022

Personally I am inclined to regard it as a good thing that you need to explicitly opt in to be open for extensions which can manipulate internal state, rather than that being the default, but this is a design question which is beyond the scope of this thread.

@rniwa
Copy link
Collaborator

rniwa commented Apr 20, 2023

F2F resolution: Closing this bug. Each custom element class needs to be designed with subclassing in mind. This is working as intended.

@rniwa rniwa closed this as completed Apr 20, 2023
@trusktr
Copy link
Contributor Author

trusktr commented Nov 15, 2023

comment moved to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants