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

Homonyms and confused names into descriptors #173

Closed
pabloalmunia opened this issue Nov 10, 2018 · 37 comments
Closed

Homonyms and confused names into descriptors #173

pabloalmunia opened this issue Nov 10, 2018 · 37 comments

Comments

@pabloalmunia
Copy link
Contributor

Homonyms and confused names into descriptors

We have found a group of homonyms (same word with different meanings) and other confused names into descriptor objects.

I have collected some opinions of my team and I include them here with humility. We are not specialists in the definition of programming language standards, we are just a team of enthusiastic programmers of this proposal.

"method"

Cases:

  • kind: "method" with a method included into value.

  • kind: "method" with a getter / setter included into descriptor.get and descriptor.set.

#162

Alternatives:

  • kind: "property" for getter / setter.
  • kind: "accessor" for getter / setter.

We vote for "property" and obtain this values for kind: "class", "method", "field" and "property".

"descriptor"

Cases:

  • descriptor or decotator descriptor is the generic name used for parameters and returns into decorator functions.

  • descriptor is a field of decorator descriptor with an Object.defineProperty() descriptor.

#162 (comment)

Alternatives:

  • remove descriptor field and include its member into the first level of decorator descriptor.
  • change the name to propertyDescriptor.

We vote for remove descriptor and change this:

{
  kind: "field"
  key: "x",
  placement: "own",
  // property descriptor for Object.defineProperty
  descriptor: { 
    configurable: false, 
    enumerable: true, 
    writable: true 
  }
}

into this:

{
  kind: "field"
  key: "x",
  placement: "own",
  configurable: false, 
  enumerable: true, 
  writable: true 
}

"initializer" vs "descriptor.value"

Cases:

  • kind: "initializer" is used for stand-alone initializer.

  • "initializer" is the field used when kind: "initializer" and include a function than is callback for side effect.

  • "initializer" is a field used when kind: "field" and include a function than return the initial value for this field.

  • The field "descriptor.value" is used when kind: "method" and include the function assigned as value.

When a programmer define a field with a method as value:

class K {
  @decorator
  prop = function () {
    return 10;
  }
}

the value is included into initializer:

{
  "kind": "field",
  "key": "prop",
  "placement": "own",
  "descriptor": {
    "configurable": true,
    "writable": true,
    "enumerable": true
  },
  "initializer": function () {
      return function () {
        return 10;
      };
    }
}

Alternatives:

  • use always initializer with a funtion than return the initial value (primitive, function, object)
  • use descriptor.value (or value) for initial values and initializer only for side effect.

We are no sure. We intuit that initializer function is important in cases where initial values can not be safely included in descriptor.value. Perhaps the solution is use alway initializer function and return a function, included when kind: "method".

@ljharb
Copy link
Member

ljharb commented Nov 10, 2018

In the spec, a property is either a data property or an accessor property - so i think that the term “property” is too generic to cover just getters/setters.

“Descriptor” is the proper generic term; whether it’s a property descriptor or a decorator descriptor seems like it should be clear from the context?

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 10, 2018 via email

@ljharb
Copy link
Member

ljharb commented Nov 10, 2018

That’s true, altho I’d argue that “method” for accessors was a confusing choice in the first place :-)

@rbuckton
Copy link
Collaborator

rbuckton commented Nov 10, 2018 via email

@littledan
Copy link
Member

@pabloalmunia Thanks for your detailed writeup and thoughts here. I appreciate how you're providing guidance on ergonomics. It sounds like the basic model of decorators worked for you, and there are just these tweaks that could make it easier to implement your own decorators; is that right?

"method"

I'm a little skeptical of using "property" to mean a getter/setter pair--as @ljharb says, lots of other programming languages use the term property that way, but in JavaScript, "data properties" are also a thing. But I think it's OK if we use more intuitive names than the spec uses internally--the JS spec is pretty hard to read.

How about "field"/"method"/"accessor"?

"descriptor"

I'm pretty convinced by your argument here. Let's unbox the property descriptor into the surrounding decorator descriptor. I found the two-level structure annoying when writing up the examples for this repository, as well.

"initializer" vs "descriptor.value"

I'm not sure about this one. Fields and methods are pretty fundamentally different here. I don't want to add extra dynamic-ness and slow-ness to methods just to make things look the same (when syntax always produces a value that's always the same, and never has a side effect when evaluated), or extra complexity to fields to give them a two-version thing (when syntax always produces an initializer, which may have a side effect, not a fixed value).

@rbuckton
Copy link
Collaborator

rbuckton commented Dec 15, 2018

I like the idea of flattening the descriptor.descriptor into a single object, and there's no real reason we need to exactly mirror the PropertyDescriptor interface in an API that isn't Object.defineProperty. That gives us additional flexibility when it comes to defining decorated class elements (i.e. could open up fields to allow Set over Define and still support @readonly, etc.).

@littledan
Copy link
Member

OK, does someone want to write a PR for this flattening?

@rbuckton
Copy link
Collaborator

Could we simplify this further and use kind: "member" for all three? A "field" is a "member" with an initializer, a "method" is a "member" with a function value, and an "accessor" is a "member" with a get and/or set. Otherwise, should we error if your kind doesn't match the attributes supplied on the descriptor?

@littledan
Copy link
Member

Initializers are optional for fields, but I imagine we could use initializer: null if we want to indicate that.

In that case, we could just omit the kind then, and use other uniquely named properties for finishers and initializers.

@nicolo-ribaudo
Copy link
Member

kind really helps implementing a decorator which can handle multiple types of class elements, since it allows us not to rely on duck typing.

if (desc.kind === "field") {
  // a field
} else if (desc.kind === "method") {
  // a method
} else if (desc.kind === "accessor") {
  // an accessor
}

if far better than

if (desc.initializer === null || desc.initializer) {
  // a field
} else if (desc.value) {
  // a method
} else if (desc.get || desc.set) {
  // an accessors
}

@pabloalmunia
Copy link
Contributor Author

Yes, kind is very important property. accessor is a good name.

@tjcrowder
Copy link
Contributor

FWIW, big +1 for

  • Using accessor instead of method for the kind on accessors
  • Flattening the element descriptor

@littledan - I can do a PR for this, though not within the next week -- probably the week after, if that would be useful.

@littledan
Copy link
Member

Thanks for laying the changes out so clearly. Two separate PRs for each of those would be great.

If you're busy next week, let's leave this task open for others to pick up (it might be me), and you can do it if you come back and it's still not done yet.

@tjcrowder
Copy link
Contributor

Flattening the element descriptor, the value property (relocated from the descriptor property) becomes oddly named. It's only allowed for methods (since fields are initialized with an initializer). Perhaps method would be clearer? Let me know people prefer value for consistency with Object.defineProperty (though since you can't specify it for fields, it's already inconsistent).

Adding in #182's changes, that would give us:

Field Value Valid For kind=
kind "field", "method", "accessor", "start", or "finish" "field", "method", "accessor", "start", "finish"
key element key (string, Symbol, or PrivateNameObject) "field", "method", "accessor"
placement "own", "prototype", or "static" "field", "method", "accessor", "start", "finish"
method the method "method"
get the getter¹ "accessor"
set the setter¹ "accessor"
enumerable true or false "field", "method", "accessor"
configurable true or false "field", "method", "accessor"
writable true or false "field", "method"
initializer undefined or function that does field init "field"
extras undefined or array of element descriptors "field", "method", "accessor"
replace² function³ "finish"
effect² function³ "start", "finish"

¹ (for kind="accessor", must have get, set, or both get and set)
² (names still being bikeshedded in #182)
³ (on kind="finish", either replace or effect may be specified, but not both)

There's also likely to be #183's kind="internal" or similar, but if I read that issue correctly it's a hand's-off object so I haven't tried to codify it above.

@littledan
Copy link
Member

Hmm, I would be fine with replacing value with method if others want to.

@Lodin
Copy link
Contributor

Lodin commented Jan 4, 2019

replace sounds great! It could solve the problem I described in my connectedCallback case. Especially if it allows not only to replace, but also to remove an element.

It should receive the replacing element as a parameter to use it's parts though. I see it in following way:

{
  key: 'connectedCallback',
  kind: 'finish',
  replace({descriptor: {value, ...descriptor}, ...propertyDescriptor}) {
    return {
      descriptor: {
         ...descriptor,
         value() {
           value.call(this);
           // do some rendering stuff
         }
      },
      ...propertyDescriptor,
    };
  }
}

Hmm, ability to change key in replace looks confusing... Not sure what to do here.

@Lodin
Copy link
Contributor

Lodin commented Jan 5, 2019

@tjcrowder, BTW, your proposal looks really great. It addresses a lot of pain points I've got after working with stage 2 decorators. Flattening the decorator is nice catch as well as replace and changing value -> method.

My 5 cents: I believe that replace should have a key allowed, because this way it will be able to pick already existing property/method and reimplement it with custom solution. If elements are going to be removed from the class decorator, it would be a nice solution to have ability to customize existing class elements. And as a parameter (as I described in my previous comment) replace should receive the element found by key. replace also allowed to return improved (or even created) version of property that replaces the existing one. It also could add an extras, I believe. Also it could return null which means that property should be simply removed.

It could look following with typescript interface:

interface ReplaceDescriptor {
  key: PropertyKey | PrivateName;
  kind: "start" | "finish";
  replace(descriptor?: Descriptor): Descriptor | null;
}

I still don't know if it would be better to allow changing key in replace or not. It looks kinda weird if decorator removes one property and adds another, but I also could imagine that it has some real usage. However, this trick could be done with replace returning null and another property created in another place (e.g. in extras), so forbidding key change looks pretty fine for me.

@littledan
Copy link
Member

The idea of the replace callback is that it would be called with the constructor, not the descriptor.

For your use case, would it work to use a method decorator on connectedCallback to annotate it when it's in the class, and a class decorator to create it when it's not provided? Would this be too ugly from an ergonomics perspective?

@tjcrowder
Copy link
Contributor

@Lodin -

Thanks! But just FWIW, this is mostly not my work, just me gathering together the work of others (along with some minor points of my own).

The question of a key on "start"/"finish' is probably best discussed over in #182 where that whole design is being discussed...? (But briefly: starters/finishes aren't specific to elements, they're specific to phases -- static, prototype, or own.)

@Lodin
Copy link
Contributor

Lodin commented Jan 5, 2019

@littledan, let me describe the project I'm working on.

I'm working on the Web Component framework based completely on the decorators. It's called Corpuscule, still in WIP, but I'm going to finish it soon. Among other packages there is @corpuscule/element that allows user to create Web Components in simple and convenient way. The basic usage is following:

@element('my-element')
class MyElement extends HTMLElement {
  @attribute('foo', Boolean)
  foo;

  @property(v => typeof v === 'string')
  bar = 'test';

  connectedCallback() {
    if (this.foo === undefined) {
      this.foo = 'Default value';
    }
  }

  [render]() {
    return html`
      <div>
        ${this.foo}
        <slot></slot>
        ${this.bar}
      </div>
    `;
  }
}

As you can see, there is no basic class, no super for connectedCallback, no noise interaction with the basic system at all. And it still follow the spec as much as possible. I even followed the naming convention creating [propertyChangedCallback] property that is the inheritor of spec's attributeChangedCallback (just for properties, not for attributes).

If we work with the basic class, as we do in the lit-element, we have two way:

  • To use spec's connectedCallback. In this case we would be forced to call super.connectedCallback() each time we want to implement it for our component. Since it is the only way to interact with Web Components lifecycle, the basic class should implement it and call some rendering stuff in there. It's quite noisy, and user can simply forget about it.
  • To use own callback: componentDidMount, first, etc. lit-element went this way, and that's good, because user won't be forced to call super method just because it is necessary for basic class.

However, I chose the third path. We have decorators, don't we? And in the current state decorators allows to re-declare method already declared in the class. So all we need is just to save the user's method, re-declare connectedCallback and call user's method from there. Spec is followed and user don't feel inconvenience.

There even is a way to extend Custom Element marked with @element decorator. Just re-declare method as own, not prototype, and you are able to extend user-defined method, not the framework one.

However, all of these approaches require access to elements. To read, to replace, to remove. I don't care if I cannot change the order, but elements should be replaceable. It allows to create wonderful things, I had a lot of pleasure working with current implementation of decorators. There were some pain points as well, but the common impression is very positive.

Well, I would agree that elements in current form is not the best solution. Working with arrays in this way is quite exhausting. I don't even think we need a direct access to the element list. I'm not sure I have the solution, but the replace in the way I described it was very interesting idea.

For your use case, would it work to use a method decorator on connectedCallback to annotate it when it's in the class, and a class decorator to create it when it's not provided? Would this be too ugly from an ergonomics perspective?

Yes, this idea may work, but it is quite noisy for user as well. It has almost no difference with calling super.connectedCallback() and allows a situation when user can forget about adding a decorator for connectedCallback, and everything just fails. Using class decorators reduces complexity, all the magic (quite simple one, actually) is hidden inside decorators.

@tjcrowder
Copy link
Contributor

@Lodin - I'm probably missing a subtlety here. :-) You still have access to elements in class decorators, that isn't changing as far as I know. Isn't that sufficient for what you're doing (which is quite cool) for connectedCallback? Perhaps in combination with a class finisher (regardless of what one ends up looking like)? If not, can you do a minimal standalone example showing your current implementation with the "old" decorators? Your repo is great, but there's a lot of moving parts there, would be good to see just the core problem you're raising.

For instance, this works (it's obviously just a quick-and-dirty) with the current @babel/plugin-proposal-decorators, and while some details may change thanks to this issue and #182, none of the functionality goes away:

function assert(condition, message = "assertion failed") {
    if (!condition) {
        throw new Error(message);
    }
}
function element(name) {
    return function elementDecorator(desc) {
        let {kind, elements} = desc;
        assert(kind === "class");
        let connectedCallbackElement = elements.find(e => e.kind === "method" && e.key === "connectedCallback");
        let superConnectedCallback;
        if (connectedCallbackElement) {
            const original = connectedCallbackElement.descriptor.value;
            connectedCallbackElement.descriptor.value = async function(...args) {
                await Promise.resolve(); // stand-in for "invalidate"
                superConnectedCallback.call(this);
                return Reflect.apply(original, this, args);
            };
        } else {
            elements.push({
                key: "connectedCallback",
                kind: "method",
                placement: "prototype",
                descriptor: {
                    async value() {
                        await Promise.resolve(); // stand-in for "invalidate"
                        return superConnectedCallback.call(this);
                    }
                }
            });
        }
        desc.finisher = function(cls) {
            superConnectedCallback = Object.getPrototypeOf(cls.prototype).connectedCallback;
        };
        desc = elements = connectedCallbackElement = undefined;
    };
}

class FakeHTMLElement {
    constructor(x) {
        this.x = x;
    }
    connectedCallback() {
        console.log(`FakeHTMLElement's connectedCallback called, x = ${this.x}`);
    }
}

@element("my-element1")
class Example1 extends FakeHTMLElement {
}

new Example1("one").connectedCallback();
// Expect:
// "FakeHTMLElement's connectedCallback called"

@element("my-element2")
class Example2 extends FakeHTMLElement {
    connectedCallback() {
        console.log(`Example2's connectedCallback called, x = ${this.x}`);
    }
}

new Example2("two").connectedCallback();
// Expect:
// "FakeHTMLElement's connectedCallback called"
// "Example2's connectedCallback called"

@Lodin
Copy link
Contributor

Lodin commented Jan 5, 2019

@tjcrowder hmm... I feel little confused. @littledan said that elements should be removed for class decorators. Do I miss something?

P.S. Thanks a lot for your idea of getting supers! That may be way better solution than mine current!

@tjcrowder
Copy link
Contributor

@Lodin - I see what you mean. So this whole discussion has nothing to do with this issue (#173), nor with #182 which is sort of mentioned in passing and which I thought you were commenting on. I have to admit I found it quite confusing, I was looking for context in the issue you posted the comment on. :-) Perhaps you could raise a new issue, if removing elements from class decorators is going to cause trouble for your project(s)? (In this specific case I don't think it does: A class finisher gets a reference to the constructor as its argument, and thus has access to the prototype via the prototype property, so the above could be accomplished -- actually, may be simpler now I think about it -- by doing it all in a class finisher. But I don't know if that's true for other elements use cases...)

@Lodin
Copy link
Contributor

Lodin commented Jan 6, 2019

@tjcrowder, yep, you're right, I feel kinda lost in all discussions here :) I'm going to create another issue.

@littledan
Copy link
Member

OK, I'm starting to write up the spec text for this change, and a few things occur to me:

  • Rather than the name effect, how about register? The idea is, you "register" the class/prototype (and maybe in the future, instance) when you're done(ish) making it. This name also corresponds to the kinds of use cases I've heard of for this callback.
  • We've discussed what order effect/register hooks and replace hooks should run in; my intuition kind of flipped when considering the name change to register--"obviously" you should register the final thing, after the replacements happen. Ultimately, I suspect we should actually just stick with what's happening with finishers today, and not sort them. This is, in some way, the most "expressive" option, since it lets programmers control what order the callbacks run in by ordering the decorators. (I expect the ordering will come up as an issue very rarely, anyway.)
  • As long as we're using unique names here (start/register/replace) that don't overlap with other names, I don't see any reason not to allow other elements to have these callbacks, just like we have with finisher today (so we can avoid the ergonomics regression). If there's an outer nested decorator on the class element, I'd say that these callbacks would run before the hooks from the other decorator runs, so we would "factor" them out into a separate kind: "hook" element which precedes the other one. We'd still want to keep a separate kind: "hook" for the case where you don't actually want there to be a class element there anymore.

I'd love to hear your thoughts on these potential changes; I'm working on writing them up, together with the above discussed changes, as specification text.

littledan added a commit that referenced this issue Jan 9, 2019
This is an ergonomics improvement, as discussed in #173
@ljharb
Copy link
Member

ljharb commented Jan 9, 2019

I'm a bit confused - anything with replace would still have to be the final item, right? It seems reasonable to have anything allow effect/register, but I'm not really sure what the advantage or use case is. It seems a lot clearer to require someone to use a "hook" kind in order to get an effect. Being able to do anything anywhere doesn't strike me as ergonomic, just convenient in the short term, but very inconvenient to maintain in the long term.

littledan added a commit that referenced this issue Jan 9, 2019
…tor (#198)

* Normative: Splat out the property descriptor in the decorator descriptor

This is an ergonomics improvement, as discussed in #173

* Fix some code and docs
@littledan
Copy link
Member

Well, if you look at the way finishers are used in example code, then forcing them to be separated out into separate extras would just be a bunch of extra boilerplate object literals. That's all. It seems just as unambiguous to permit this, Could you elaborate on your concerns about maintenance?

@littledan
Copy link
Member

I'm a bit confused - anything with replace would still have to be the final item, right?

To clarify on this point, right now you can have many finishers registered, and each finisher can either return undefined or a new class. They run in an order based on who asked for them, and they will run based on the class returned from the previous one, or the one before that if the previous one returned undefined. I'm proposing that we keep doing that.

@ljharb
Copy link
Member

ljharb commented Jan 10, 2019

ah - i was under the impression that it wouldn't make sense for a decorator to run on "not the item it was typed next to", but i guess it also makes sense that if i typed two decorators, and the first replaced the item, that it'd be reasonable to assume that i (the class author) intended that behavior.

It might help if you could show me (or link to) pairs of examples, where one side has them separate and another condensed.

@littledan
Copy link
Member

I am not suggesting anything that is really different from how finishers work in the current spec, where the finisher is then not passed to the next decorator. The only difference is how it shows up in the elements array.

@littledan
Copy link
Member

Oh, I realized a big flaw with this idea to allow start/register/replace callbacks together with the element: Frequently, you'll want a static register callback on top of an own/prototype class element. This will still have to be an extra. So if the ergonomics improvement is gone in the most common case, probably best to not include this complexity.

@tjcrowder
Copy link
Contributor

Rather than the name effect, how about register?

I thought we'd decided to change effect to finish in #182 (comment). I like the orthogonality of start/finish. While some use cases may use it for registering the result, others will use it for other purposes (for instance, modifying the passed-in object rather than replacing it). FWIW, I'd suggest sticking with finish. It's generic.

We've discussed what order effect/register hooks and replace hooks should run in; my intuition kind of flipped when considering the name change to register--"obviously" you should register the final thing, after the replacements happen.

That makes sense. (And goes well with the name finish. :-) )

Ultimately, I suspect we should actually just stick with what's happening with finishers today, and not sort them.

If the programmer includes both finish and replace, we need to pick an order for them (and your thought putting finish last makes good sense to me). But the programmer can choose a different order by adding a separate element; we wouldn't reorder elements, just say what order callbacks on the same element will be called in.

As long as we're using unique names here (start/register/replace) that don't overlap with other names, I don't see any reason not to allow other elements to have these callbacks...

I think you've already decided not to do this for other reasons, but in case you're still thinking about it: While I like avoiding having to add elements, I would worry a bit about the semantics. Adding a replace callback to a field element feels off: it's not going to replace the field. As callbacks for a hook they make sense, but they feel misplaced on other kinds of elements.

@ljharb
Copy link
Member

ljharb commented Jan 10, 2019

I think it’s very helpful to make one word require a replacement be provided, and a different word disallow one.

@tjcrowder
Copy link
Contributor

@ljharb - Agreed. The suggestion in #182 (comment) was that start and finish can't return a replacement, and replace must.

@littledan
Copy link
Member

Do you think a hook with the same placement and multiple callbacks is something that will come up very often? I wonder if we should restrict it to one, just so we don't have to specify the relative order of replace/finish

@tjcrowder
Copy link
Contributor

I'm afraid I don't have a feel for that. But if I needed to specify more than one, I know it would irk me slightly if I couldn't. My gut is that specifying start and either finish or replace wouldn't be surprising, but that specifying both replace and finish would be fairly surprising (they run at the same stage in the process, so why specify both?). Since it's only the order of finish vs. replace that's in question, perhaps just restrict it to either finish or replace (but allow start as well). E.g., start+finish is okay, start+replace is okay, finish+replace isn't.

But for me, allowing all three and specifying the order (replace then finish) makes more sense. It could be done in a follow-up, but...

@littledan
Copy link
Member

I think it's not necessary to allow finish and replaced together, as replace subsumes finish--you can just put all your finish logic in replace. We don't have to decide on any nontriviak relative ordering logic for start vs replace, so it seems fine to allow that combination.

littledan added a commit that referenced this issue Jan 10, 2019
littledan added a commit that referenced this issue Jan 10, 2019
* Normative: Rename kind: "initializer" to kind: "hook"

* Normative: Rename initializer

For hooks, rename to start
For fields, rename to initialize
Keep the internal slot [[Initializer]], as class
fields uses that name

* Normative: Replace finisher with replace, finish hooks

As discussed in #173
@littledan littledan mentioned this issue Jan 15, 2019
6 tasks
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

7 participants