WICG / webcomponents

Web Components specifications
Other
4.36k stars 370 forks source link

How to define APIs only for custom element authors #758

Closed tkent-google closed 5 years ago

tkent-google commented 6 years ago

The following issues need APIs which should be used by custom element authors and should not be used by custom element users.

At the March F2F we came up with one idea. However using ShadowRoot for such APIs looks very weird to me and I'd like to discuss it again.

Candidates:

IMO, the second one or the third one is much better than the first one.

@annevk @domenic @rniwa @trusktr What do you think?

[1] If a custom element implementation stores a ShadowRoot / Something instance to this.foo_, a custom element user can get the instance by accessing yourElement.foo_. There are some techniques to avoid such casual access.

rniwa commented 5 years ago

Hm... I guess another option is make HTMLElement's constructor take an argument. So you do:

class MyElement extends HTMLElement {
    constructor() {
        const myInternal = new ElementInternal;
        super(myInternal);
    }
}

That's pretty funky. It would mean that ElementInternal (I don't think it's a good name but for the purpose of discussing the semantics, I'd use this name) would have a pre-attachment / pre-construction state too. I guess element.attachElementInternal() which would throw on a second invocation would be fine... that would be analogous to how shadow DOM API would work. Custom elements that don't use it can either attach it and never use it, or we could make this an opt-in feature where you'd have to declare it in a static field like we did for observedAttributes.

tkent-google commented 5 years ago

I talked to JavaScript-binding experts in the team, and our conclusion was it's very difficult to detect whether an operation is inside new MyElement() or not. Also, I noticed it's difficult to define createdCallback behavior in new MyElement() case.

I think the second approach (new ElementInternals(this), this.attachInternals(), customElements.createInternals(this), or something) is acceptable though it's impossible to prevent non-ce-authors from using it perfectly. We may clear can-create-internals-flag in create-an-element path and upgrade-an-element path, and may clear it when the 'custom'-state element is connected.

tkent-google commented 5 years ago

or we could make this an opt-in feature where you'd have to declare it in a static field like we did for observedAttributes.

Opt-in sounds a good idea. Adding a flag to ElementDefinitionOptions would be an easy way.

customElements.define('my-element', MyElement, {needsElementInternals: true});
// ==> ElementInternals creation is allowed.

customElements.define('my-element', MyElement);
// ==> ElementInternals creation is disallowed.
caridy commented 5 years ago

if we want new XElement() to continue to work, I can't see how we'd be able to turn on access to the thing only inside constructors.

I think I'm fine with that. Let's not bend backward for this feature, and keep the new XElement() intact.

to prevent non-ce-authors from using it perfectly

I agree with @tkent-google, it should be fine, just like it is fine for attachShadow. It you claim it during the construction process, no one else can claim it after that.

Opt-in sounds a good idea. Adding a flag to ElementDefinitionOptions would be an easy way.

@tkent-google no, it is not. In many cases, an author produces a component, and the consumer of that component is responsible for registering, and decide what tagName to use. This model works pretty well for us today, and having to have a coordination between the registerer and the author is going to be pretty bad. The static field as the opt-in described by @rniwa is more realistic since it is described by the author.

I guess element.attachElementInternal() which would throw on a second invocation would be fine

@rniwa I think that breaks any possibility for multiple classes in your hierarchy to tap into the internals. It is not a deal breaker IMO, but will require some coordination by the subclass to wrap the call to attachElementInternal is it needs access to the internals, but I do agree that this is probably our best option so far.

annevk commented 5 years ago

@caridy don't you need such coordination anyway given styling proposals and such that are underway?

domenic commented 5 years ago

Anther side effect of the new callback: the possibility to bypass internals defined during the inheritance.

This is true with any of the options on the table. E.g. you say that the Something constructor is guaranteed to be untouched, but that's not true; the subclass does not need to call it (they can use Reflect.construct to directly call HTMLElement, and bypass the superclass constructor). In the end the author of the custom element class has ultimate control.

Hm... I guess another option is make HTMLElement's constructor take an argument.

Interesting. This seems workable to me, although a bit weird for developers. Especially because standing advice for JS class usage is "don't do anything before calling super()".

Basically ElementInternal is useless until it's passed to a [HTMLConstructor] via super(), then it becomes useful. That's not too complicated.

I think this works, although there may be Reflect.construct shenanigans that could mess it up.

I guess element.attachElementInternal() which would throw on a second invocation would be fine... that would be analogous to how shadow DOM API would work. Custom elements that don't use it can either attach it and never use it, or we could make this an opt-in feature where you'd have to declare it in a static field like we did for observedAttributes.

The problem there is you can attach it to arbitrary elements you don't own. Even if we only allow it on custom elements, that seems unfortunate.

Also, I noticed it's difficult to define createdCallback behavior in new MyElement() case.

I think you just define it to happen at the bottom of the [HTMLConstructor] steps. (So it happens when you call super(), before the rest of the author-defined constructor code.)

Opt-in sounds a good idea. Adding a flag to ElementDefinitionOptions would be an easy way.

So I guess the idea here is that this reduces the bad case to when custom element authors opt-in with needsElementInternals: true, but fail to do this.attachInternals() (or whatever) before calling any author code. That helps a bit.


Overall I think we have three proposals that each have OK tradeoffs:

I personally think the createdCallback() version is simplest, especially because I'd anticipate authors only using either createdCallback() or the constructor, not both. But all of these seem livable.

I think it would be good to spend pre-TPAC time keeping our minds open and fleshing out the list of alternatives, either by adding more or expanding on the pro-cons. We've had a lot of good brainstorming over the last week that feels productive. We can then narrow the field at TPAC.

rniwa commented 5 years ago

@domenic : for createdCallback, I'd add another con that invoking the subclass' method while executing super() in that subclass' constructor is a very strange programming pattern.

class MyElement {
    constructor() {
        super(); // This calls createdCallback
        this._myStateObject = ~;
    }
    createdCallback(elementInternal) {
        // Can't see this. _myStateObject.
    }
}

We have to do this for the same reason that we can't tell the end of the constructor. An alternative is to enqueue a custom element reaction after the constructor had run but then it would mean that the constructor won't have access to ElementInternal even though that's the most natural place to set the default tab index, etc...

tkent-google commented 5 years ago

Some code examples and comments.

class MyElement extends HTMLElement {
    constructor() {
        const myInternals = new ElementInternals();
        // myInternals is inactive.  All operations on myInternals throw exceptions.
        super(myInternals);
        // myInternals is active.
    }
}

Custom element users can steal ElementInternals instance by inheritance.

Code by a custom element author:

class MyElement extends HTMLElement {
  createdCallback(internals) {
    this.internals_ = internals;
  }
}

Code by a custom element user:

class MyElement2 extends MyElement {
  createdCallback(internals) {
    // This is called instead of MyElement's one.
    super.createdCallback(intrenals);
  }
}
class MyElement extends HTMLElement {
  // static getter is better than define()'s option?
  static get needsElementInternals() { return true; }

  constructor() {
    super();
    this.internals_ = this.attachInternals();
    // attachInternals() throws if needsElementInternals doesn't exist or returns false.
  }
}
annevk commented 5 years ago

During the F2F the point was made for the third option that static is likely better than define() due to it working for inheritance.

annevk commented 5 years ago

I made the point that maybe getInternals() is a better name, since if you subclass a builtin it'll have some non-default state stored there already.

rniwa commented 5 years ago

Rough consensus at TPAC F2F: We'll tentatively use attachInternals with static field indicating that the element internals should exist for a given custom element (so that it would work for subclasses) where calling it twice will throw but @domenic will consult with TC39 to see if there is a generic protected field mechanism that we should be using here.

caridy commented 5 years ago

Excellent, options 3 sounds promising, and should work fine when providing abstractions on top of HTMLElement, which is what we do mostly.

domenic commented 5 years ago

TC39 cross-check posted at https://github.com/tc39/ecma262/issues/1341; stay tuned.

littledan commented 5 years ago

The approach you settled on here sounds fine for now, if you need to get something in soon, but if you can wait a little bit for decorators, then we might have an option which is a little more ergonomic: https://github.com/tc39/ecma262/issues/1341#issuecomment-435487021

domenic commented 5 years ago

So, reporting back from the TC39 thread in a bit more detail. A few takeaways:

Taking all this advice in to account, my proposed solution is that we go with attachInternals() but no needsInternals. Thus, attachInternals() becomes similar to the (closed) shadow DOM model:

This also has the benefit of only adding one API, so that in some glorious decorators future, we can explain the decorator as sugar over one old API, instead of two, and we don't have to worry about interactions (e.g. what if someone uses the decorator + needsInternals?).

What do folks think?

caridy commented 5 years ago

@domenic I'm fine with that. Just having a precedent on attachShadow will definitely help to explain this new API. As a component author, if you care about it, you can just call it during construction, if you extend a class that cares about it, well, you have to coordinate it, just like attachShadow.

littledan commented 5 years ago

About the timing of decorators: my current plan is to propose it for Stage 3 in January 2019. If this is successful, I hope we could get to the point of decorators shipping in browsers by late-2019/mid-2020 (given that multiple browsers have been expressing support for the decorators proposal). If this schedule would get interfere with your timeline, and if it's OK if there isn't any strong protection guarantee, Domenic's solution sounds fine to me.

rniwa commented 5 years ago

I don't think we'd be okay with attachInternal being callable without the knowledge of the custom element itself. @saambarati is following up on that thread but it doesn't seem like a good idea to implement any interim solution until there is a general consensus / agreement as to how a protected state is being done in TC39.

annevk commented 5 years ago

Requiring needsInternals might still be a good way of allowing component developers to opt into this explicitly and not require all existing components to retroactively start calling attachInternals() early enough to avoid being bamboozled. That'd avoid @rniwa's concern as well.

rniwa commented 5 years ago

Right, requiring needsInternals would address my concern.

littledan commented 5 years ago

needsInternals sounds like a sound solution if it's passed as an option to customElements.define, but I guess it leaves two issues, that it's strange to have to mention this in two places, and the difficulty of working with subclasses. And if you make your class needsInternals accidentally, this opens you up to anyone calling attachInternals.

Here's a slightly different possibility, as an analogue of this decorator-based solution . At least it solves the last issue (which might not be so serious).

// In the browser
customElements.getAttachInternals = function(klass) {
  if (klass is already registered as a custom element) throw new TypeError;
  return function(element) {
    // Possibly generalize this check to make working with subclasses better
    if (element's custom element definition's constructor !== klass) throw new TypeError;
    return element.[[Internals]];
  };
}

// Usage example
class MyElement extends HTMLElement {
  #internals = attachInternals(this);
}
let attachInternals = customElements.getAttachInternals(MyElement);
customElements.define('my-el', MyElement);

By requiring that getAttachInternals be called before customElements.define, the system ensures that this call is collaborating with the code which defines the element (if custom element classes don't sit around un-defined for long). You could later make getAttachInternalsbe overloaded as a decorator, to save a little bit of code, but no need to block on that.

domenic commented 5 years ago

@rniwa @annevk can you say why you are OK with attachShadow({ mode: 'closed' }) being called without the knowledge of the custom element itself, but not OK with the same for attachInternals()?

The reasons I think needsInternals is subpar are:

I think there is consensus/agreement on how protected state is done in TC39; it's not built in to the language, but decorators provide a powerful enough mechanism to enable patterns like protected, friend, .NET-style internal, etc.

annevk commented 5 years ago

@domenic well, existing components are aware of attachShadow(), but not attachInternals(). (And observing such a static at define() time would be consistent with observedAttributes et al, no?)

domenic commented 5 years ago

existing components are aware of attachShadow(), but not attachInternals()

So this is just a staging problem? I.e. needsInternals is an API to work around the fact that we didn't introduce all web components APIs into the world at the same time? That seems like an unfortunate way of designing APIs...

And observing such a static at define() time would be consistent with observedAttributes et al, no?

I'm not sure what this is in reference to.

domenic commented 5 years ago

Some further discussion in IRC: https://freenode.logbot.info/whatwg/20181108#c1817142

caridy commented 5 years ago

I could not agree more with @domenic, you're trying to make this tamper-proof, but there are so many ways to go around it (e.g.: get the constructor, change proto, etc.) that I don't think it is worth it. I don't think that's the goal of the web-component APIs. Instead, you can provide those low-level primitives that people can use, and bend, and do crazy stuff with it, and that is the way-of-the-web.

rniwa commented 5 years ago

I think it was a mistake that attachShadow was allowed on a custom element without an opt-in. The fact anyone can add a shadow root on a custom element is highly undesirable. The existence of this mistake shouldn't encourage us to make more mistakes.

annevk commented 5 years ago

Another way to slice this might be to offer a way to opt-out of encapsulated features, a static unneeded taking ["shadow", "internals"], which would make attachShadow() and getInternals() (or attachInternals() if there's some kind of cascade going on) throw, similar to how they throw for the html element.

(I don't think anyone is trying to make this secure/tamper-proof as that is indeed not possible; this is about the encapsulation boundary.)

domenic commented 5 years ago

I really like @annevk's idea. A way of preventing unexpected uses without having to say "give me an X, even though I will do nothing with it" makes a lot of sense. It leads to more readable code and seems easier to optimize.

tkent-google commented 5 years ago

@rniwa , what do you think about the opt-out idea?

My opinion is that either of opt-in, opt-out, nothing is acceptable. I'd like to proceed this anyway :-)

rniwa commented 5 years ago

Opt-out seems good especially since it would also work for shadow root but unneeded doesn't seem like a good name though. Maybe disable? We probably need to come up with a slightly longer name since it would be a static class variable, and we'd want to be mindful of existing custom elements which may or may not have such a static class variable.

annevk commented 5 years ago

Paint color I'd be okayish with: disableEncapsulation = ["shadow", "internals"]. (I also considered disableFeatures and disableFunctionality, which I'm meh on, but could live with too. unneeded also seems fine though and probably unique enough given it's a little odd.)

rniwa commented 5 years ago

I don't think disableEncapsulation makes much sense because we're making attachShadow throw. disabling encapsulation on surface sounds like something you'd do to expose internals to the outside.

disableFeatures is a lot better name IMO although I think we can do better. We should probably name it disabledFeatures since this isn't really a method. Or maybe unusedFeatures?

tkent-google commented 5 years ago

+1 todisabledFeatures.

trusktr commented 5 years ago

Hey you all, but this attachInternals and disabledFeatures business implies there is a set of features that is pre-defined by the browser vendors, and does not let library authors define them. Furthermore, something like a disabledFeatures blacklist would not make sense with an unknown set of features provided by library authors.

Mixins are better in the sense that you can get them from anywhere (as builtins, or from libraries) and they are all opt-in.

Can we try to keep the API space symmetric between builtin and userland?


On another note,

protected-like methods from a mixin

Here's an idea on how to expose a feature from a mixin to a CE author, while letting the CE author decide to (or not to) expose the API to public space.

Suppose the mixin accepts a key. Then inside the module or closure where the CE class is defined, we can create the mixin:

const key = Symbol() // key can be any reference, like WeakMap keys.
const WithSauce = makeWithSauce(key)

then extend from it (assume the mixin extends HTMLElement by default):

const key = Symbol() // key can be any reference, like WeakMap keys.
const WithSauce = makeWithSauce(key)
export default class SaucyEl extends WithSauce() {}

Note we do not export the key.

Now to use a method createSauce provided by the mixin, we can pass the key in:

const key = Symbol() // key can be any reference, like WeakMap keys.
const WithSauce = makeWithSauce(key)
export default class SaucyEl extends WithSauce() {
  connectedCallback() {
    this.createSauce(key) // it works!
  }
}

Now suppose a user of the element tries to call the method on an instance of the element. The user's code runs outside the module scope where SaucyEl was defined, so

import SaucyEl from 'saucy-lib'
customElements.define('saucy-el', SaucyEl)
const saucyEl = document.querySelector('saucy-el')

saucyEl.createSauce() // throws an Error

const key = Symbol()
saucyEl.createSauce(key) // throws an Error

To expose the feature for public use, the library author just has to expose the key, or provide a public proxy method that doesn't require a key.

This technique is "protected-like", because a subclass (SaucyEl) is able to use the method from a parent class (the WithSauce mixin). But it is also private-like because the SaucyEl subclass has the key, but subclasses of SaucyEl don't, so the method is sort of private for subclasses of SaucyEl.

tkent-google commented 5 years ago

@trusktr thank you for the idea.

then extend from it (assume the mixin extends HTMLElement by default):

The mixin needs to support not only HTMLElement but also any classes extending HTMLElement as a base class. I have two concerns about it.

domenic commented 5 years ago

I don't think we should be contemplating such unusual patterns for the web platform; we should stick with normal class hierarchies.

trusktr commented 5 years ago

@tkent-google

I can't imagine how to implement it in Blink with V8 though I'm a Chrome engineer.

Isn't there a feature of Chrome where new features can be implemented in JavaScript before they are (if ever) converted to C/C++? I've seen the debugger pause on such builtin features before. And, I'm just guessing, but if they can be implemented in JS, could the same mechanics be used on the native side?

Okay, assuming implementation details work out fine, is there anything bad about the pattern itself?

Why can't WebIDL define such a sort of feature? Is it because it is language specific (JS has this ability)?

In that sense, then aren't classes also language-specific and thus we shouldn't use them (f.e. Custom Elements)?

trusktr commented 5 years ago

we should stick with normal class hierarchies.

What constitutes a normal hierarchy?

Can't we say that after we've mixed a mixin class into a new class definition, that the new definition is now a normal hierarchy? It works just like other hierarchies, plus we have things like Symbol.hasInstace to make instanceof work fine, etc.

By the way, I'm not absolutely married to the idea, but so far I like it. I'm curious to know why it would be a bad pattern compared to others.

trusktr commented 5 years ago

Hell you all, I just want to reiterate that I feel passing options like needsInternal or default styles (re: https://github.com/w3c/webcomponents/issues/468) to customElements.define seems out of place because:

For this reason I think that mechanisms like @decorators, static props, and Mixins(). are better. The author can decide what APIs a class needs, what styles it has, what behaviors it has, etc.

The end user just needs to define the element, and use it.

So in my opinion, authors of the custom elements should use class infrastructure to its fullest rather than putting parts of the class into customElements.define calls.

Author:

import styled from 'lib1'
import withRender from 'lib2'
import html from 'lib3'
import attribute from 'lib5'
// or const {attribute} = CustomElements // builtin
const [One, Two, Three] = window.ElementFeatures // builtin

@styled({
  foo: {
    color: 'skyblue'
  }
})
class AwesomeElement extends withRender(HTMLElement) {
  static elementInternals = [One, Two, Three]
  static userAgentStyle = new window.CSSStyleSheet( ... )

  @attribute('material-color', Color)
  materialColor = new Color('red')

  render() {
    return html`<div class="${this.classes.foo}"></div>`
  }
}

End user:

import AwesomeElement from 'awesome-element'
customElements.define('awesome-el', AwesomeElement)
document.body.innerHTML = `<awesome-el material-color="pink"></awesome-el>`

But then again, the author could abstract it away.

Author:

// adding onto the same file:
export const define = name => {
  customElements.define(name, AwesomeElement, {
    userAgentStyle: new window.CSSStyleSheet( ... ),
    elementInternals: [One, Two, Three]
  })
}

End user:

import define from 'awesome-element'
define('awesome-el')
document.body.innerHTML = `<awesome-el></awesome-el>`

I guess abstracting works, but now its an extra step.

If I'm already using class to define my class, why not just use class to define everything about my class? 😃

trusktr commented 5 years ago

The only reason why customElements.define even needs to accept a name is presumably to allow the end user to customize this important aspect of an element. Otherwise, all we need is

class MyEl extends HTMLElement {
  static get elementName() { return "my-el" }
}

customElements.define(MyEl)

So unless it is critical for an end user to change something, then I feel it doesn't need to go into customElements.define because we already have class.

trusktr commented 5 years ago

But then again, isn't an end user able to change stuff by extending from a class?

F.e., a class can easily be extended by an end user and a static elementName value overridden. It just seems like if we're using class, maybe we should be sticking class specifics inside the class definition rather than passing class specifics to an API outside of the class definition.

class MyEl extends HTMLElement {
  static get elementName() { return "my-el" }
}

class NewEl extends MyEl {
  static get elementName() { return "new-el" }
}

customElements.define(MyEl) // defines <my-el> element
customElements.define(NewEl) // defines <new-el> element

What would be the downside of having class-specifics inside the class definitions instead passing the specifics to customElements.define?

I can see passing the name into customElements.define makes it easy to do a simple re-name without having to write an additional class ... extends ... { static elementName = '...' }, but for other stuff like styles and API features, I think the class definition is there for that!

tkent-google commented 5 years ago

As for ElementInternals, we made consensus around TPAC 2018. Though I haven't received any feedbacks from TAG, a specification PR was already approved, and we'll merge it with a PR for form-associated custom element.

justinfagnani commented 5 years ago

Does anyone have an ElementInternals explainer?

tkent-google commented 5 years ago

@justinfagnani AFAIK, Form Participation API Explained is the only document other than the HTML specification.

annevk commented 5 years ago

In due course we should probably add an introduction section to the HTML Standard.

frank-dspeed commented 4 years ago

Maybe we should simply point out that customElements are not a magic thing without the define stuff and all that they are

//CustomElement based on HTMLUnknownElement
const myCustomElement = document.createElement('my-element')
//Then apply some lhooks or what ever to it
myCustomElement.connectedCallback = ()=> {
  this.innerHTML = '<p>Iam a Complex Application </p>'
}
document.body.append(myCustomElement)

// We can also use existing HTML Elements
const MYCustomElment = document.getElementById('my-element')
MYCustomElment.connectedCallback = () => {
  this.innerHTML = '<p>Iam a Complex Application </p>'
}
document.body.append(myCustomElement)

Now the leaved out question is how does get connectedCallback get called ? you can do it manual or via mutationObserve and register it then you can also attach more behavior or bindings its a customElement so your free

as long as you understand a CustomElement is a modifyed version of a HTMLElement Instance.

trusktr commented 10 months ago

I wrote a new issue describing how an internals callback would be a lot better than the current this.attachInternals(), and also proposed some ideas that could be doable when decorators land in browsers. These ideas keep internals fully protected: