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

[WIP] Private Class Methods Refactor #1

Conversation

tim-mc
Copy link

@tim-mc tim-mc commented Nov 14, 2018

This is a refactor of babel#8654 that implements private method support on top of babel#8130.

@tim-mc
Copy link
Author

tim-mc commented Nov 14, 2018

@nicolo-ribaudo

@tim-mc tim-mc force-pushed the private-class-methods-stage-3-refactor branch 2 times, most recently from 32b5a65 to 93f2a89 Compare November 14, 2018 21:44
@nicolo-ribaudo
Copy link
Owner

nicolo-ribaudo commented Nov 14, 2018

I think that private methods should be transpiled exactly like private methods, because I could overwrite them. e.g.

class A {
  #x() { return 2 }
}

// ...

let _xMethod = function () { return 2 };

class A {
  constructor() {
    _x.set(this, {
      writable: true,
      value: _xMethod
    });
  }

}

var _x = new WeakMap();

This is because methods could be overwritten exactly as if they were fields, e.g.

class A {
  #x() { return 2 }

  setX() { this.#x = "foo"; }
}

.gitignore Outdated Show resolved Hide resolved
@tim-mc
Copy link
Author

tim-mc commented Nov 14, 2018

@nicolo-ribaudo re: #1 (comment), private methods are not writable. Your second code snippet should result in a TypeError.

@nicolo-ribaudo
Copy link
Owner

Oh right. I still think that we should use weakmaps with descriptors, since decorators (I will implement support for decorated private elements after that this PR is merged) can make private methods writable, or change them to be fields at runtime. Having a single way of accessing private things makes it a lot easier.

@tim-mc
Copy link
Author

tim-mc commented Nov 15, 2018

WeakMap versus WeakSet was a discussion we had 1-2 months ago with the team that is implementing private methods in TypeScript as well as the group that is behind the class fields proposal that's making it way through TC39 (e.g., @littledan). It also came up in my original PR for private methods back in September, which led to me rewriting the implementation (the original version of which actually did use WeakMaps) in that same PR about a month ago.

The main factor behind rewriting the WeakMap implementation to use WeakSet instead was that this is closer to the spec - private method descriptors have configurable: false and writable: false.

IIRC the performance cost of using WeakMap over WeakSet was an additional motivation behind rewriting the original implementation to end up with the current WeakSet output.

Given the above, is it possible to continue with the WeakSet implementation in conjunction with decorators? I'm not familiar enough with the decorator transform to know what level of complexity this introduces.

@tim-mc tim-mc force-pushed the private-class-methods-stage-3-refactor branch from 9651072 to 1d08d59 Compare November 15, 2018 02:09
@littledan
Copy link

I think we could use WeakSet for private methods by themselves, and move to WeakMap if needed when decorators are in use. It is very unfortunate to have per-instance, per-method space overhead! The WeakSet representation has the potential to eliminate this by using the same set for a group of methods.

@nicolo-ribaudo
Copy link
Owner

Ok we can go ahead with weak sets. If we see that having to have a different path for decorators makes things too complex we can always revisit the decision later.

@tim-mc tim-mc changed the title [WIP] Private Class Methods [WIP] Private Class Methods Refactor Nov 15, 2018
Copy link
Owner

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this plugin throw when it finds a static private method or a private accessor, since they are not supported?

const { params, body } = prop.node;
const methodValue = prop.node
? t.functionExpression(methodId, params, body)
: prop.scope.buildUndefinedNode();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can this be undefined?

const {
staticNodes,
instanceNodes,
declarationNodes,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't declarationNodes go with staticNodes?

parserOpts.plugins.push(
"classProperties",
"classPrivateProperties",
"classPrivateMethods",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a @babel/plugin-syntax-private-methods for this, parallel to @babel/plugin-proposal-private-methods.

@tim-mc tim-mc closed this Nov 24, 2018
nicolo-ribaudo added a commit that referenced this pull request Feb 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants