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

Fix #4840: Alias class prototype for methods in loose mode #5560

Merged
merged 2 commits into from Aug 27, 2017

Conversation

@oliverdon
Copy link
Contributor

oliverdon commented Mar 28, 2017

Q A
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? no
Spec Compliancy? yes?
Tests Added/Pass? yes
Fixed Tickets Fixes #4840
License MIT
Doc PR no
Dependency Changes no

Alias class prototype for methods in loose mode.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Mar 28, 2017

@oliverdon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @existentialism, @loganfsmyth and @hzoo to be potential reviewers.

if (!this.aliasInserted) {
const classProto = t.memberExpression(this.classRef, t.identifier("prototype"));
const protoDeclaration = t.variableDeclaration("var", [
t.variableDeclarator(t.identifier(this.methodAlias), classProto),

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 20, 2017

Member

This needs to be a unique identifier, not the known I'd _proto.

This comment has been minimized.

Copy link
@oliverdon

oliverdon Jul 21, 2017

Author Contributor

Thanks, that makes sense.

would replacing line 10:

this.methodAlias = "_proto";

with

this.methodAlias = this.path.scope.generateUidIdentifier("proto").name;

be a good way of achieving this?

This comment has been minimized.

Copy link
@jridgewell

This comment has been minimized.

Copy link
@oliverdon

oliverdon Jul 21, 2017

Author Contributor

Made the change, and caught up changes from 7.0.

@oliverdon oliverdon force-pushed the oliverdon:loose-class-reuse-prototype branch from 57f02e7 to 56fd0c6 Jul 21, 2017
t.identifier("prototype"),
);
const protoDeclaration = t.variableDeclaration("var", [
t.variableDeclarator(t.identifier(this.methodAlias), classProto),

This comment has been minimized.

Copy link
@hzoo

hzoo Jul 21, 2017

Member

If you are going to use generateUidIdentifier, then you should just the identifier part and not do the .name 😄

this.methodAlias = this.path.scope.generateUidIdentifier("proto")
t.variableDeclarator(this.methodAlias, classProto),

This comment has been minimized.

Copy link
@oliverdon

oliverdon Jul 21, 2017

Author Contributor

Ah, good point 😅, done.

@oliverdon oliverdon force-pushed the oliverdon:loose-class-reuse-prototype branch from 56fd0c6 to 53268dc Jul 21, 2017
@@ -6,6 +6,25 @@ export default class LooseClassTransformer extends VanillaTransformer {
constructor() {
super(...arguments);
this.isLoose = true;
this.aliasInserted = false;
this.methodAlias = this.path.scope.generateUidIdentifier("proto");

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jul 21, 2017

Member

Nit: this uses a global name. We can instead test if methodAlias is set yet, and create it when needed.

This comment has been minimized.

Copy link
@oliverdon

oliverdon Jul 21, 2017

Author Contributor

Good point, removed this from constructor.

@oliverdon oliverdon force-pushed the oliverdon:loose-class-reuse-prototype branch 2 times, most recently from 1897e78 to ff11dce Jul 21, 2017
@jridgewell jridgewell force-pushed the oliverdon:loose-class-reuse-prototype branch from ff11dce to c54ae28 Aug 26, 2017
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Aug 26, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4709/

@jridgewell jridgewell merged commit 960151c into babel:7.0 Aug 27, 2017
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.05% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
GulajavaMinistudio added a commit to GulajavaMinistudio/babel that referenced this pull request Aug 27, 2017
Fix babel#4840: Alias class prototype for methods in loose mode (babel#5560)
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Aug 27, 2017

@oliverdon thanks for your efforts! awesome stuff.

^ aside: btw we should figure out plugin options (loose mode) for the repl

@hzoo hzoo mentioned this pull request Sep 16, 2017
2 of 2 tasks complete
@lock lock bot added the outdated label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.