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

Bug-fix: export default decorated class parsed as class expression #7189

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

nveenjain
Copy link
Contributor

Q                       A
Fixed Issues? Fixes #7123
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR No
Any Dependency Changes? No
License MIT

Now decorators are checked just after checking for exported classes, then they are processed and then class is processed.

@nicolo-ribaudo
Copy link
Member

@nveenjain As you can see by travis-ci, the output of a fixture of a babel plugin changed. If you delete the output.js file and run make test-only, it will be updated automaticaly.

@babel-bot
Copy link
Collaborator

babel-bot commented Jan 10, 2018

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

@nveenjain
Copy link
Contributor Author

@nicolo-ribaudo Fixed that. It took some time as i was struggling with rebasing.

} else if (this.match(tt.at)) {
this.parseDecorators(false);
// startNode has changed
return this.parseClass(this.startNode(), true, false, false, true);
Copy link
Member

Choose a reason for hiding this comment

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

Q: Why do we need to this.startNode() instead of using expr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think expr will still point to the node which is of decorators. After parsing decorators, current node will point to class Node which is needed to be parsed.

Copy link
Member

Choose a reason for hiding this comment

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

The parseDecorator function (called by parseDecorators) creates its own node, so expr won't be used. If you have tried and using expr breaks something leave it as it is now, otherwise don't create an unnecessary node 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it creates its own node which according to me, is required.
For example

export default @dec class x{}

In the code above expr will point to the node having start at 15 (when expr is assigned then current node is still tt.at) so when class is parsed without creating an extra node, it's current start will be pointed by 15 which acc. to me should be 20. that's why we need to create extra Node, which will point to current location of class i.e. 20. I've seen that class declaration/expression nodes start at the starting of class keyword. I maybe wrong though.. 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo, i cross-checked by using expr instead of creating a new node, and tested it for following input:-

export default @dec class A {}

, i got this output:-

[ Node {
    type: 'ExportDefaultDeclaration',
    start: 0,
    end: 30,
    loc: SourceLocation { start: [Object], end: [Object] },
    declaration:
     Node {
       type: 'ClassDeclaration',
       start: 15,
       end: 30,
       loc: [Object],
       decorators: [Array],
       id: [Object],
       superClass: null,
       body: [Object] } } ]

However the ouptut must be :-

[ Node {
    type: 'ExportDefaultDeclaration',
    start: 0,
    end: 30,
    loc: SourceLocation { start: [Object], end: [Object] },
    declaration:
     Node {
       type: 'ClassDeclaration',
       start: 20,
       end: 30,
       loc: [Object],
       decorators: [Array],
       id: [Object],
       superClass: null,
       body: [Object] } } ]

I think we need to create new node. However, suggestions/corrections are appreciated 🙂

Copy link
Member

@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.

I tested and decorators are included in the class location:

"use strict";

const babylon = require("./packages/babylon");

const code = "@deco class Foo {}";

const ast = babylon.parse(code, { plugins: ["decorators2"] });

console.log(JSON.stringify(ast.program.body[0], null, 2));
{
  "type": "ClassDeclaration",
  "start": 0,
  "end": 18,
  "loc": {
    "start": {
      "line": 1,
      "column": 0
    },
    "end": {
      "line": 1,
      "column": 18
    }
  },
  "decorators": [
    {
      "type": "Decorator",
      "start": 0,
      "end": 5,
      "loc": {
        "start": {
          "line": 1,
          "column": 0
        },
        "end": {
          "line": 1,
          "column": 5
        }
      },
      "expression": {
        "type": "Identifier",
        "start": 1,
        "end": 5,
        "loc": {
          "start": {
            "line": 1,
            "column": 1
          },
          "end": {
            "line": 1,
            "column": 5
          },
          "identifierName": "deco"
        },
        "name": "deco"
      }
    }
  ],
  "id": {
    "type": "Identifier",
    "start": 12,
    "end": 15,
    "loc": {
      "start": {
        "line": 1,
        "column": 12
      },
      "end": {
        "line": 1,
        "column": 15
      },
      "identifierName": "Foo"
    },
    "name": "Foo"
  },
  "superClass": null,
  "body": {
    "type": "ClassBody",
    "start": 16,
    "end": 18,
    "loc": {
      "start": {
        "line": 1,
        "column": 16
      },
      "end": {
        "line": 1,
        "column": 18
      }
    },
    "body": []
  }
}

@nveenjain
Copy link
Contributor Author

Updated the PR 😃 . Thanks, got to learn something while doing this.

@@ -1368,6 +1368,9 @@ export default class StatementParser extends ExpressionParser {
return this.parseFunction(expr, true, false, true, true);
} else if (this.match(tt._class)) {
return this.parseClass(expr, true, true);
} else if (this.match(tt.at)) {
this.parseDecorators(false);
return this.parseClass(expr, true, false, false, true);
Copy link
Member

Choose a reason for hiding this comment

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

Last thing, you passed too many parameters here. It should be expr, true, true like the line above.

@nveenjain
Copy link
Contributor Author

@nicolo-ribaudo , sorry for bothering, but is anything still wrong in PR?

Copy link
Member

@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.

It's good 🙂

@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jan 30, 2018
@hzoo hzoo merged commit 65ae4ff into babel:master Jan 30, 2018
@nveenjain nveenjain deleted the fix7123 branch January 30, 2018 17:25
aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants