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

How to define APIs only for custom element authors #758

Open
tkent-google opened this Issue Jul 30, 2018 · 87 comments

Comments

Projects
None yet
@tkent-google
Copy link

tkent-google commented Jul 30, 2018

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:

  • ShadowRoot
    Pros: Usually only custom element implementation knows the instance of ShadowRoot which is used to implement the custom element. It's difficult for custom element users to get a ShadowRoot instance created by a custom element implementation [1]
    Cons: ShadowRoot is an interface for tree-encapsulation. Adding features unrelated to tree-encapsulation looks like a design defect. We should not make ShadowRoot a kitchen sink.
    Cons: Not all custom element implementations need ShadowRoot. For example, a checkbox custom element won't need ShadowRoot. Creating unnecessary ShadowRoot for such APIs is not reasonable.
    Cons: If a custom element implementation uses no ShadowRoot, a custom element user can call element.attachShadow() to get a ShadowRoot instance, and thus get access to these private APIs.

  • new Something(customElement)
    It throws if users try to create it for the same element twice. It throws if the constructor is called with non-custom elements. The interface Something is called as ElementStates in #738, and called as HTMLElementPrimitives in #187.
    Pros: ShadowRoot won't have unrelated APIs.
    Pros: Usually only custom element implementation knows the instance of Something. It's difficult for custom element users to get a Something instance created by a custom element implementation [1]
    Cons: If a custom element implementation uses no Something, a custom element user can call new Something(element) successfully to get a Something instance.

  • Deliver a Something instance by a custom element callback
    See #187 (comment) for more details.
    Pros: ShadowRoot won't have unrelated APIs.
    Pros: Custom element users can not create a Something instance unlike other two candidates.
    Pros: It's difficult for custom element users to get a Something instance delivered to a custom element [1]
    Cons: UA implementation would need larger code for a new callback, compared to the other two 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.

@caridy

This comment has been minimized.

Copy link

caridy commented Jul 30, 2018

I have a similar realization the other day when polyfilling shadowRoot.ariaLabel and co., which can fall into the same category. shadowRoot becoming a kitchen sink will be unfortunately, but considering that WC APIs are very low level, it might be ok.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 31, 2018

I don't quite understand why delivering Something via a callback is more expensive than new Something(). (A way to avoid the "cons" of new Something() is perhaps to allow declaration ahead of time that the custom element won't use it, or indeed, require explicit opt-in that it's going to be used.)

(Another "cons" of ShadowRoot is that it's not limited to custom elements.)

@alice

This comment has been minimized.

Copy link
Contributor

alice commented Aug 2, 2018

I'm writing up the properties @caridy mentioned.

The design problem we're trying to solve there is:

  • We need a way to set non-reflected versions of the ARIA IDL attributes which will change dynamically
    • e.g. ariaChecked, which represents the current "checked" state
  • These versions should be shadowed by the reflected version
    • e.g. if I set ariaChecked on the host element, as well as the shadow root, the version on the host element should override whatever was set on the shadow root - but then if I delete the ariaChecked property on the host, it should revert to using the version on the shadow root

ShadowRoot really seems like a good fit for this problem in some ways, since it's analogous to using :host in a <style> element within the shadow root - but unlike style, the properties on the host element can only be set directly on the host element (i.e. can not be defined anywhere but directly on the affected element), so there's no logical place to put the :host equivalent.

It also means that you can encapsulate semantic information about the host element without needing to register a custom element.

However, I can imagine asking the question "why can't I set shadowed versions of any element property on the shadow root?" - which definitely sets us on a road to a kitchen sink scenario.

Also, if your custom element doesn't otherwise need a shadow root, it seems like a shame to need to attach one for this purpose.

We could theoretically create a new type of object to hold semantic properties, analogous to a constructable stylesheet, but unlike constructable stylesheets there's no precedent for the type of object this would need to be - it's really just a simple, small map of properties.

Does anyone have any ideas for alternatives to using ShadowRoot here?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 2, 2018

@alice @tkent-google's post has a number of alternatives, no? We'd need a pick a better name than Something, but otherwise one of those approaches would work I think.

@alice

This comment has been minimized.

Copy link
Contributor

alice commented Aug 2, 2018

@annevk I should have addressed those better, sorry!

Here are my concerns with the those approaches (I count only two, really - the first alternative is just to use ShadowRoot):

  • Either way, we'll have to create a new type which is simply an empty object with AccessibilityRole and AriaAttributes mixed in, which is kind of awkward.
  • The "con" noted for the new Something(element) option seems like a deal breaker to me (that anyone can construct one for an element which doesn't already have one) - it's the opposite of encapsulation, which is what we're aiming for. (Presumably constructing a Something for a built-in element would be forbidden). Do authors need to construct a new Something defensively whenever they create a custom element, for all possible values of Something (if many APIs use this strategy)?
  • The lifecycle callback option is probably the one I would be least uncomfortable with, but I think it implies having a single object named something like ElementConfiguration which can grow to hold all of these various objects (ElementStates, HTMLElementPrimitives, etc.) which is passed in in a single lifecycle event. Otherwise, it seems like either each new API has to create a new lifecycle event (?) or else the createdCallback ends up with a mess of arguments.
@trusktr

This comment has been minimized.

Copy link

trusktr commented Aug 2, 2018

but considering that WC APIs are very low level, it might be ok.

That may be an overstatement. Web devs want to make components, so if they move off React or Vue (or etc) to Web Components, they will use ShadowDOM. It's not very low level, it's a normal part of organizing web UI. Any serious web developer should be expected to organize code with components of nested trees; it's standard.

I'm in favor not to use it as a kitchen sink for component parts. Rather, ShadowDOM is it self a component part, and should keep it's concerns specific. Other parts can have other jobs.


@tkent-google About new Something (element), what's that idea? Is that a suggestion for an entity-component system?

@alice

This comment has been minimized.

Copy link
Contributor

alice commented Aug 6, 2018

@annevk and @tkent-google I just raised WICG/aom#127 to try and avoid further rabbit-holing here on this specific issue, but I'd appreciate your thoughts over there if you have time!

@alice

This comment has been minimized.

Copy link
Contributor

alice commented Aug 6, 2018

One benefit of the createdCallback (presumably?) option is that, if we do combine all possible settings into something like ElementConfiguration, the API gives you an opportunity to learn by exploration of that object.

With new MyThing(element), authors would have to learn about each MyThing separately (although obviously good docs can help with this).

Also, if new MyThings keep being added, custom elements which were written before they existed run the risk of having MyThings being defined by the page instead. This may not be a concern, but it seems awkward to me.

@trusktr

This comment has been minimized.

Copy link

trusktr commented Aug 13, 2018

@tkent-google About new Something (element), what's that idea?

Oops, nevermind, it was based on the idea of new ElementStates(this) which you linked to. Sorry for the noise.

Deliver a Something instance by a custom element callback

I think that's my favorite idea.

I added an example using that idea here: #738 (comment)

To avoid enabling all features (and avoid runtime cost) maybe they can be opt-in? f.e.,

import Privates from './privates-helper'
const _ = new Privates

class MyEl extends HTMLElement {
  useFeatures(features) {
    _(this).cssStates = features.use('cssstates')
    // or
    _(this).cssStates = features.cssStates()
  }

  connectedCallback() {
    // blink every second
    setInterval(() => {
      _(this).cssStates.has('enabled') ?
        _(this).cssStates.remove('enabled') :
        _(this).cssStates.add('enabled')
    }, 1000)
  }
}

We could achieve this example with CSS, but this example is similar to how a div element doesn't add a hover class for you, but instead a :hover state, so it doesn't interfere with the div user's class list.

@hayatoito

This comment has been minimized.

Copy link
Member

hayatoito commented Aug 14, 2018

@tkent-google,

Deliver a Something instance by a custom element callback

It looks this idea is the most favored. Something might become yet another kitchen-sink, but I think that would be much better than using ShadowRoot here.

Could you have a chance to make the idea more concrete one or prototype?
Several new features are waiting for this idea, I guess.

@tkent-google

This comment has been minimized.

Copy link

tkent-google commented Aug 15, 2018

Many people are favor of the third one. Let's proceed with the third one if no one has a strong opinion against it.

We need to define the followings:

Interface name

HTMLElementPrimitives (drop HTML?), ElementSemantics, etc.
Please reply your ideas.

Callback

First I thought we could add a Something argument to connectedCallback, but I found that custom element implementations needed to call a protected API before connecting to a document tree in Form Participation API. So we should introduce new callback.

My current proposal is createdCallback, which is called just after upgrade, maybe before attributeChangedCallback.

@tkent-google

This comment has been minimized.

Copy link

tkent-google commented Aug 15, 2018

@annevk

I don't quite understand why delivering Something via a callback is more expensive than new Something().

I guess adding a new custom element callback requires larger code in UA than allowing new Something(element) just once. We already have four callbacks, and adding another would not be a big issue.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 15, 2018

Another name idea: ElementInternals (after internal slots). Without HTML seems reasonable as we'd reuse this if we ever added custom elements in other namespaces. createdCallback also seems reasonable, though a bit unfortunate we then have both that and a constructor.

@caridy

This comment has been minimized.

Copy link

caridy commented Aug 15, 2018

ElementInternals is definitely better than ElementSemantics, +1 on that.

@alice

This comment has been minimized.

Copy link
Contributor

alice commented Aug 15, 2018

Would we put all of the properties on what I've called ElementSemantics on ElementInternals, or would we have a semantics object hanging off ElementInternals like I proposed (I referred to ElementInternals as ElementConfiguration as a placeholder in that doc, and in my comments above)?

@caridy

This comment has been minimized.

Copy link

caridy commented Aug 15, 2018

@alice I think a flatten structure is just fine. HTMLElement itself is mostly flatten anyways.

@trusktr

This comment has been minimized.

Copy link

trusktr commented Aug 20, 2018

Extending from option 3, could the following help with performance?

Option 4: static list of features.

What if elements could specify specific features to use in a static prop, so the engine doesn't unnecessarily have to pass every single feature into a callback even if an element won't use each feature?

F.e.

class Foo extends HTMLElement {
  static get observedAttributes() { ... }
  static get features() { return [ ... ] }
}

Then the features could do what they want: specify new callbacks, or provide certain instance props, etc.

Either it would be up to the feature how it is exposed, or perhaps they all get injected into a standard callback like option 3's createdCallback (though I think a different name would be better because we already have constructor).

As for the static list of features, if they were strings,

  static get features() { return [ 'builtin-feature', 'user-feature' ] }

it would be easiest, but with the problem of name clashing (suppose we let anyone provide features, not built-in features):

  static get features() { return [ BuiltinFeature, UserFeature ] }

References would avoid name clashing, and not require a registry:

But then, this seems like mixins!

Option 5: class-factory mixins

Could mixins do the trick?

class Foo extends BuiltinFeature( UserFeature( HTMLElement ) ) {

As a real-world example, SkateJS is a web component library whose features are consumable as mixins, making them easy to mix and match.

@tkent-google

This comment has been minimized.

Copy link

tkent-google commented Aug 30, 2018

ElementInternals sounds good. If no one objects it, let's adopt it.

@alice @caridy I also suppose flatten structure. ElementInternals interface has attributes/operations for various features such as accessibility, form controls, ...

@trusktr I'm not sure I understand what you wrote correctly. Probably such feature list isn't necessary because we assume a single ElementInternals instance handles all features for the associated element?

Re: callback name
createdCallback represents the timing when it is called, and doesn't represent the purpose. We may give more descriptive name like elementInternalsCreated receiveElementInternals.

@trusktr

This comment has been minimized.

Copy link

trusktr commented Sep 2, 2018

@tkent-google

because we assume a single ElementInternals instance handles all features for the associated element?

That's what I was trying to avoid with the idea of a list or mixins, because otherwise it means ElementInternals will include all possible features even if the features are not going to be used.


In Option 5 above, class-factory mixins provide a way to opt-in to using specific features (and avoid resource waste by creating the feature instances only when needed):

import UserFeature from 'npm-package'
const {BuiltinFeature1, BuiltinFeature2} = customElements.elementFeatures

class MyEl extends BuiltinFeature1( BuiltinFeature2( UserFeature( HTMLElement ) ) ) {
  // ...
}

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

In that sample, the class will have only the features it has specified.


If the list gets long, then

const Features = BuiltinFeature1( BuiltinFeature2( UserFeature( HTMLElement ) ) )

class MyEl extends Features {
  // ...
}
@trusktr

This comment has been minimized.

Copy link

trusktr commented Sep 2, 2018

@alice An example of option 5 for element semantics would be like

// ...
const {withSemantics} = customElements.elementFeatures

class MyEl extends withSemantics( HTMLElement ) {
  // ...
  receiveSemantics( semantics ) {
    const privates = elementPrivates.get(this);
    privates.semantics = semantics;
  }
}

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

where in this example, the receiveSemantics callback will be called by the withSemantics mixin implementation.

The withSemantics implementation might call the receiveSemantics method in its constructor, for example.

The implementation of withSemantics might look like the following (except it may not be JavaScript):

window.customElements.elementFeatures.withSemantics = function withSemantics(Base) {
  return class WithSemantics extends Base {
    constructor() {
      super()
      if (typeof this.receiveSemantics === 'function') {
        const semantics = ___; // create the semantics for `this` element
        this.receiveSemantics( semantics )
      }
    }
  }
}

It could also be placed on window.elementFeatures, but that creates a new window global. Seems like customElements would be a good fit for this.

@trusktr

This comment has been minimized.

Copy link

trusktr commented Sep 2, 2018

Another example could be observing children. A mixin could make this an opt-in feature:

const {withChildren} = customElements.elementFeatures

class MyEl extends withChildren( HTMLElement ) {

  // This callback is provided by the `withChildren` mixin.
  childrenChangedCallback() {

    // Work with children here, don't worry about
    // if children exist in `connectedCallback`.

    // Not only will this fire whenever children change, but
    // also once after `connectedCallback` has been called.

    // Maybe children here are guaranteed to be already
    // upgraded if they are custom elements?

  }

}
@tkent-google

This comment has been minimized.

Copy link

tkent-google commented Sep 11, 2018

@trusktr, with your ideas, can we realize APIs which are available only for custom element authors?

@trusktr

This comment has been minimized.

Copy link

trusktr commented Sep 15, 2018

can we realize APIs which are available only for custom element authors

Are you wondering if we can allow the mixins to be used only with Element classes? If so, then yeah we can:

The mixin could check the base class to make sure it is an Element class, f.e., pseudo code of the native code in JS:

function withAwesomeness(Base = HTMLElement) {
  if (!extendsFrom(Base, Element)) throw new TypeError('mixin can only be used on classes that extend from Element')

  // continue, return class extends Base...
}

Or did you mean something else?

@domenic

This comment has been minimized.

Copy link
Contributor

domenic commented Nov 2, 2018

TC39 cross-check posted at tc39/ecma262#1341; stay tuned.

@littledan

This comment has been minimized.

Copy link

littledan commented Nov 2, 2018

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: tc39/ecma262#1341 (comment)

@domenic

This comment has been minimized.

Copy link
Contributor

domenic commented Nov 7, 2018

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

  • There may be an elegant future solution with decorators. However, decorators are a "stage 2" feature and move very slowly through the standards process; indeed at stage 2 it's unclear whether they will be implemented at all. So we should proceed with an interim solution, and in some years maybe decorators will be available and we can consider that. We should not take on a dependency.
  • At the same time, we may want to contemplate whether there are features of our interim solution that we could tweak to make it more upgradeable to a decorator future.
  • TC39 provided some useful feedback on our existing solution at tc39/ecma262#1341 (comment) which I think we should take into account. In particular it seems like subclassing does not work well with our existing solution, so the needsInternals placement on the class instead of as an option to customElements.define() is a bit questionable now.
  • Again about subclassing, there was the insight that basically if you want subclassing to work, you're going to lose the protection of disallowing arbitrary consumers from attaching ElementInternals. This insight emerged in tc39/ecma262#1341 (comment).

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:

  • It's somewhat "uncoupled" from custom elements. The main coupling is that we restrict it to only be callable on custom element instances, just like attachShadow({ mode: 'closed' }) is only callable on custom elements + a safelist of elements.
  • If a custom element doesn't call attachShadow({ mode: 'closed' })/attachInternals() in its constructor, then any consumer of that element can call the method and start messing with the element's shadow tree/internals. Oh well.
  • If a custom element does call attachShadow({ mode: 'closed' })/attachInternals() in its constructor, then subclasses are screwed. (I.e., they need to coordinate with their superclass if they want access to the shadow root/internals.)

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

This comment has been minimized.

Copy link

caridy commented Nov 7, 2018

@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

This comment has been minimized.

Copy link

littledan commented Nov 7, 2018

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

This comment has been minimized.

Copy link
Contributor

rniwa commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Member

annevk commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

rniwa commented Nov 8, 2018

Right, requiring needsInternals would address my concern.

@littledan

This comment has been minimized.

Copy link

littledan commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

domenic commented Nov 8, 2018

@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:

  • It adds more surface area to obsolete in a decorator-friendly future. In that future (and I don't share @littledan's optimism on the timeline, but it may indeed happen eventually), we could straightforwardly explain @HTMLElement.internals as sugar on top of attachInternals(). But explaining its interactions with a two-API feature is harder. E.g. what happens if you use @HTMLElement.internals + needsInternals together, or a base class uses one but not the other.
  • To make it tamper-proof we have to move it to customElements.define(), or do a custom prototype walk that checks own properties until it hits a built-in element prototype. The former option conflicts with @caridy's desire to separate class definition from registration, and the latter option is really weird and unprecedented in both JS and web specs. On the other hand, if we don't make it tamper-proof, it doesn't have any real benefits. Which leads me to just omitting it.

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

This comment has been minimized.

Copy link
Member

annevk commented Nov 8, 2018

@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

This comment has been minimized.

Copy link
Contributor

domenic commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

domenic commented Nov 8, 2018

@caridy

This comment has been minimized.

Copy link

caridy commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

rniwa commented Nov 9, 2018

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

This comment has been minimized.

Copy link
Member

annevk commented Nov 9, 2018

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

This comment has been minimized.

Copy link
Contributor

domenic commented Nov 9, 2018

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

This comment has been minimized.

Copy link

tkent-google commented Nov 15, 2018

@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

This comment has been minimized.

Copy link
Contributor

rniwa commented Nov 15, 2018

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.

@tkent-google tkent-google referenced this issue Nov 15, 2018

Open

Form Participation API #305

3 of 5 tasks complete
@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 15, 2018

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

This comment has been minimized.

Copy link
Contributor

rniwa commented Nov 15, 2018

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

This comment has been minimized.

Copy link

tkent-google commented Nov 16, 2018

+1 todisabledFeatures.

@trusktr

This comment has been minimized.

Copy link

trusktr commented Nov 19, 2018

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

This comment has been minimized.

Copy link

tkent-google commented Nov 19, 2018

@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.

  • Hard to standardize it because WebIDL is not capable to define such mixin.
  • Implementability is questionable. Usually web platform features are implemented at outside of a JavaScript engine. I'm not sure if all UAs can provide such dynamic mixins with their JavaScript engines. Actually I can't imagine how to implement it in Blink with V8 though I'm a Chrome engineer.

aarongable pushed a commit to chromium/chromium that referenced this issue Nov 19, 2018

Form-associated custom elements: Implement HTMLElement.prototype.atta…
…chInternals().

- Introduce empty ElementInternals interface
- Add HTMLElement.prototype.attachInternals()
- Add a flag to ElementRareData about ElementInternals attachment
- Promote "ElementInternals" runtime flag to "test"

All the change is behind the runtime flag.
Some test cases in external/wpt/custom-elements/CustomElementRegistry.html
fail by this CL because this CL enables GET() operation for "disabledFeatures",
and these test cases doesn't expect it.  We should update the testcases later.


Bug: 905922
Bug: w3c/webcomponents#758
Change-Id: I7b3dbfd1d422c7eae5c7a6e01ddea50a00134a6e
Reviewed-on: https://chromium-review.googlesource.com/c/1341734
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609231}
@domenic

This comment has been minimized.

Copy link
Contributor

domenic commented Nov 19, 2018

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

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