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

Normative updates to the spec, March 2023 #499

Closed
pzuraq opened this issue Mar 7, 2023 · 7 comments
Closed

Normative updates to the spec, March 2023 #499

pzuraq opened this issue Mar 7, 2023 · 7 comments

Comments

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 7, 2023

This issue is a meta issue for tracking a number of normative updates that are being proposed to the spec in the upcoming March plenary.

  1. Remove the dynamic assignment of [[HomeObject]] from decorator method application/evaluation (PR, Issue)

    This dynamic assignment/update of a function's [[HomeObject]] is a new behavior that has not occurred before in the spec, and is even asserted against. It results in unexpected behavior and does not have an actual use-case. This decision was likely due to a misunderstanding of what the MakeMethods purpose was. This change removes the reassignment entirely.

  2. Call decorators with their natural this value instead of undefined (PR, Issue)

    Currently decorators are always called with an undefined this value, even when it might appear that they should have one (e.g. @foo.bar should be called with foo as the receiver). This change threads the receiver through so the decorator can be called with it.

  3. Throw an error if the value passed to addInitializer is not callable (PR)

    addInitializer expects to receive a function, and all of the spec text downstream from it assumes this as well, so this assertion is necessary to ensure that the value is actually callable.

  4. Set the name of the addInitializer function (PR)

    Sets the name of addInitializer, which is currently an empty string.

  5. Remove SetFunctionName from decoration (PR)

    Currently, SetFunctionName is called on the returned decorated methods. This two main problems:

    1. It messes up stack traces, because the actual function name will be different than the code, which would be confusing when trying to debug.
    2. SetFunctionName asserts against ever being called twice for the same method.
  6. "Bind" static accessors directly to the class itself. (PR pending, issue)

    Static accessors are implemented conceptually as a sugar on top of private fields. A getter and setter are generated which access private storage on the same class. For static accessors, this results in them not working when inherited by a child class, since static fields and accessors are not redefined on child classes.

The proposed update is to "bind" static accessors to their original class, e.g. instead of being sugar for:

class A {
  static #x = 42
  static get x() { return this.#x }
  static set x(v) { this.#x = v }
}

They would desugar to:

class A {
  static #x = 42
  static get x() { return A.#x }
  static set x(v) { A.#x = v }
}
@rbuckton
Copy link
Collaborator

5. Remove assert from SetFunctionName so function names can be updated (PR)

I'm not sure I agree with this one, since you can return a shared function:

function noop() {}
function conditional(cond) {
  return function (target, context) {
    return cond ? target : noop;
  };
}

class C {
  @conditional(true) static a() {}
  @conditional(false) static b() {}
  @conditional(false) static c() {}
}

console.log(C.a.name, C.b.name, C.c.name);

Without this change, this would log a noop noop. After this change, this would log a c c, and would affect every class using @conditional(false), not just the local one.

There's no way to tell whether the returned function is safe to be renamed in this fashion. I'd much rather someone explicitly do this instead if changing the name is truly important:

function dec(m) {
  function x() {
    return m.call(this, ...arguments);
  }
  Object.defineProperty(x, "name", {
    ...Object.getOwnPropertyDescriptor(x, "name"),
    value: typeof m.name === "symbol" ? `[${m.name.toString()}]` : m.name
  });
  return x;
}

Yes, it's a bit more cumbersome, but it's also more predictable.

@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 13, 2023

I think I'm coming around to that with more discussion that's been going on, I was worried this would result in a lot of problematic debugging behavior but it does seem like changing the name would actually mess with stack traces even more (see discussion here: #502)

@Jamesernator
Copy link

Probably should include static accessors on this list as well.

@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 21, 2023

All of these changes gained consensus in the plenary today, I will be merging them into the updated spec as soon as I have time

@Amareis
Copy link

Amareis commented Apr 11, 2023

Hello, @pzuraq, can you at least describe final consensus? Seems like this issues blocks at least esbuild implementation, which is blocks all users of vite from using new decorators...

@pzuraq
Copy link
Collaborator Author

pzuraq commented Apr 11, 2023

As noted above, all of the proposed changes were accepted. I’ve been very busy lately so haven’t had time to update the spec, but I’ll try to get to it today.

@pzuraq
Copy link
Collaborator Author

pzuraq commented Apr 12, 2023

All updates have been merged. For update 6, see the linked PR for more details(comment)

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

4 participants