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

Class decorators don't respect field declarations order #183

Closed
nicolo-ribaudo opened this issue Dec 13, 2018 · 6 comments
Closed

Class decorators don't respect field declarations order #183

nicolo-ribaudo opened this issue Dec 13, 2018 · 6 comments

Comments

@nicolo-ribaudo
Copy link
Member

Suppose we have the following class:

class A {
  foo = alert(1);
  #bar = alert(2);
  baz = alert(3);
}

new A();

It correctly alerts 1, 2, 3

Lets add a class decorator:

function noop() {}

@noop
class A {
  foo = alert(1);
  #bar = alert(2);
  baz = alert(3);
}

new A();

Now it alerts 1, 3, 2.

This is because DecorateConstructor recreates the list of element descriptors by joining a list containing only the public elements with a list containing only the private elements.

We can fix this problem in two ways:

  1. Always run public initializers first, and then private initializers. This is the easiest way but it can be confusing.
  2. Add some kind of "placeholders" in the elements array, so that class decorators can't read or modify them but they can be kept in the correct order. The noop would then take a parameter similar to
{
  kind: "class",
  elements: [
    {
      kind: "field",
      placement: "own",
      key: "foo",
      initializer() { alert(1); }
    },
    PrivateElementPlaceholder { [[PrivateName]]: #bar },
    {
      kind: "field",
      placement: "own",
      key: "baz",
      initializer() { alert(3); }
    }
  ]
}
@littledan
Copy link
Member

Good point!

What if we represent private field initializers as "starter" elements?

@nicolo-ribaudo
Copy link
Member Author

By doing so a class decorator would be able to change their placement.

Actually I'm not even sure that we should pass starter/finisher elements to class decorators, since they are too "opaque" to be possible for a decorator to do something useful with them (similarly to #177 (comment)).

@littledan
Copy link
Member

Hmm, good point. So those should also be "placeholders" (since starters are interspersed with other initializers)?

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Dec 13, 2018

Yes.
Probably an object with { kind: "placeholder" }* and an internal slot pointing to the descriptor would be ok.
And it should probably throw if they are added/removed from elements.

* kind: placeholder/internal/obfuscated?

@littledan
Copy link
Member

I like "internal", but further bikeshedding still welcome on this thread. Interested in making a PR for this, @nicolo-ribaudo ?

littledan added a commit that referenced this issue Jan 11, 2019
Initially, the decorators proposal had class descriptors which
contained an elements array with public and private elements.
Later, concerns were raised about exposing this private information
unexpectedly (#133), and this access was removed (#151).

However, this patch caused a problem: private field initializers
were accidentally sorted after public field initializers, an observable
and counterintuitive change (#183). Halfway solutions (#184) did
not really work out; we added back the full element support
in #133 (comment) .

Although we believe that it makes sense for decorators to be able
to see into class bodies, including private, and there are many
important use cases (for both public and private, #192), this support
doesn't have strict consensus in committee, so this patch serves
as a conservative option in case including elements is unacceptable.

This is not the preferred alternative, but it's a well-articulated
backup plan. I'm writing this out in advance in the interest of time,
to ensure that we have a path to Stage 3 and that this does not block
things.
@littledan
Copy link
Member

We considered this change in #184, but realized that it would be really complicated and ultimately insufficient. In the end, we added back in private elements to class elements (#197), and many of us prefer that solution, but if it's unacceptable, we have #203 as a backup plan. In either of those two cases, class decorators respect field evaluation order.

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

2 participants