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

Error when default exporting a decorated class #2702

Closed
JacksonGariety opened this Issue Oct 31, 2015 · 20 comments

Comments

Projects
None yet
@JacksonGariety

JacksonGariety commented Oct 31, 2015

This works as expected:

@fooDecorator
export class Foo {...}

This does not:

@fooDecorator
export default class Foo {...}

The error thrown is:

Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got "Decorator"

The latter snippet is valid es7 syntax right now, right?

@eelkeh

This comment has been minimized.

Show comment
Hide comment
@eelkeh

eelkeh Oct 31, 2015

Might be related, with:

function decorator(cls) {
  return cls;
}

@decorator
class B {
  constructor() {
    return;
  }
}

and babel --presets stage-0 test.js seems to be throwing the same error.

eelkeh commented Oct 31, 2015

Might be related, with:

function decorator(cls) {
  return cls;
}

@decorator
class B {
  constructor() {
    return;
  }
}

and babel --presets stage-0 test.js seems to be throwing the same error.

@JacksonGariety

This comment has been minimized.

Show comment
Hide comment
@JacksonGariety

JacksonGariety Oct 31, 2015

@eelkeh I am using the es2015 and react presets and most of the plugins in stage 0 but not all.

JacksonGariety commented Oct 31, 2015

@eelkeh I am using the es2015 and react presets and most of the plugins in stage 0 but not all.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 31, 2015

Member

Not sure if it's the same exact issue but decorators are not working atm #2645

Member

hzoo commented Oct 31, 2015

Not sure if it's the same exact issue but decorators are not working atm #2645

@eelkeh

This comment has been minimized.

Show comment
Hide comment
@eelkeh

eelkeh Oct 31, 2015

@JacksonGariety yes I guess I still need the es2015 preset, with babel --presets es2015,stage-0 test.js and export default class I get the same error.

eelkeh commented Oct 31, 2015

@JacksonGariety yes I guess I still need the es2015 preset, with babel --presets es2015,stage-0 test.js and export default class I get the same error.

@shuhei shuhei referenced this issue Oct 31, 2015

Merged

Babel 6 #7

7 of 7 tasks complete
@resistdesign

This comment has been minimized.

Show comment
Hide comment
@resistdesign

resistdesign Nov 5, 2015

Same issue here with decorated classes.

"devDependencies": {
    "babel-core": "^6.1.2",
    "babel-preset-stage-0": "^6.1.2"
  }

resistdesign commented Nov 5, 2015

Same issue here with decorated classes.

"devDependencies": {
    "babel-core": "^6.1.2",
    "babel-preset-stage-0": "^6.1.2"
  }
@diegoddox

This comment has been minimized.

Show comment
Hide comment
@diegoddox

diegoddox Nov 11, 2015

Same for me.

"devDependencies": {
    "babel-core": "^6.1.4",
    "babel-preset-stage-0": "^6.1.2",
  }

diegoddox commented Nov 11, 2015

Same for me.

"devDependencies": {
    "babel-core": "^6.1.4",
    "babel-preset-stage-0": "^6.1.2",
  }
@alexbeletsky

This comment has been minimized.

Show comment
Hide comment
@alexbeletsky

alexbeletsky Nov 11, 2015

Same for me.

  "devDependencies": {
    "babel-core": "^6.1.2",
    "babel-preset-es2015": "^6.1.2",
    "babel-preset-react": "^6.1.2",
    "babel-preset-stage-0": "^6.1.2",

alexbeletsky commented Nov 11, 2015

Same for me.

  "devDependencies": {
    "babel-core": "^6.1.2",
    "babel-preset-es2015": "^6.1.2",
    "babel-preset-react": "^6.1.2",
    "babel-preset-stage-0": "^6.1.2",
@geekyme

This comment has been minimized.

Show comment
Hide comment
@geekyme

geekyme Nov 12, 2015

same for me

    "babel-cli": "6.1.4",
    "babel-core": "6.1.4",
    "babel-loader": "6.1.0",
    "babel-preset-es2015": "6.1.4",
    "babel-preset-react": "6.1.4",
    "babel-preset-stage-0": "6.1.2",
    "babel-runtime": "6.0.14",

geekyme commented Nov 12, 2015

same for me

    "babel-cli": "6.1.4",
    "babel-core": "6.1.4",
    "babel-loader": "6.1.0",
    "babel-preset-es2015": "6.1.4",
    "babel-preset-react": "6.1.4",
    "babel-preset-stage-0": "6.1.2",
    "babel-runtime": "6.0.14",
@pavelbinar

This comment has been minimized.

Show comment
Hide comment
@pavelbinar

pavelbinar commented Nov 12, 2015

+1

3 similar comments
@dpwrussell

This comment has been minimized.

Show comment
Hide comment
@dpwrussell

dpwrussell commented Nov 12, 2015

+1

@rzschoch

This comment has been minimized.

Show comment
Hide comment
@rzschoch

rzschoch commented Nov 12, 2015

+1

@rthewhite

This comment has been minimized.

Show comment
Hide comment
@rthewhite

rthewhite commented Nov 12, 2015

+1

@LookLikeAPro

This comment has been minimized.

Show comment
Hide comment
@LookLikeAPro

LookLikeAPro commented Nov 12, 2015

+ 1

@ChrisCinelli

This comment has been minimized.

Show comment
Hide comment
@ChrisCinelli

ChrisCinelli Nov 12, 2015

Is this a general problem with Babel 6, or just an edge case using the decorator before export ?

I started getting:

  npm WARN deprecated babel-core@5.8.29: Babel 5 is no longer being maintained. Upgrade to Babel 6. 

ChrisCinelli commented Nov 12, 2015

Is this a general problem with Babel 6, or just an edge case using the decorator before export ?

I started getting:

  npm WARN deprecated babel-core@5.8.29: Babel 5 is no longer being maintained. Upgrade to Babel 6. 
@daiheitan

This comment has been minimized.

Show comment
Hide comment
@daiheitan

daiheitan commented Nov 13, 2015

+1

1 similar comment
@wizhippo

This comment has been minimized.

Show comment
Hide comment
@wizhippo

wizhippo commented Nov 13, 2015

+1

@shuhei

This comment has been minimized.

Show comment
Hide comment
@shuhei

shuhei Nov 13, 2015

Member

Please stop +1ing. I want only meaningful updates.

Member

shuhei commented Nov 13, 2015

Please stop +1ing. I want only meaningful updates.

@daiheitan

This comment has been minimized.

Show comment
Hide comment
@daiheitan

daiheitan Nov 13, 2015

I think this is related to #2645 , just have to wait for the Decorator fix.

daiheitan commented Nov 13, 2015

I think this is related to #2645 , just have to wait for the Decorator fix.

@f3rno

This comment has been minimized.

Show comment
Hide comment
@f3rno

f3rno commented Nov 15, 2015

+1

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Sep 7, 2016

Collaborator

Comment originally made by @kmalakoff on 2015-11-23T13:54:25.000Z

--------------------------INVESTIGATIONS--------------------------

I've taken a look at the code and stepped through the tests after hoping that the comment "Decorators are not supported yet in 6.x pending proposal update." meant that decorators had been implemented in Babel 6, but were disabled due to proposal compliance reasons.

It looks like the feature was only partially implemented (you'll see it looks like it was started and then stopped mid-implementation) and only a small subset of the tests were transferred from Babel 5 (and then the tests were deleted). For example:

I believe there is probably one to three days of work to get the old implementation of decorators working with all of the [[ https://github.com/babel/babel/tree/5.x/packages/babel/test/fixtures/transformation/es7.decorators | previous tests ]].

To port the tests, it will require a bit of a steep learning curve into babel internals because decorators need to re-write a few very different scenarios of syntax (class vs property decorators) which can include switching the order of expressions in the AST, injecting new expressions, etc. After stepping through the code, I think the current, partially-implemented version may only give an indication of the final solution, but may require a bit of a rework to get all of the scenarios working.

--------------------------TRIAGE DECISION--------------------------

I don't know what the proposed spec changes are, but I believe this feature is not being actively developed pending the proposal (TBC) so we should agree the triage path while the spec gets worked out. Some potential options:

  1. "The purist": investigate the proposed spec changes and update the tests to match the latest spec and complete the port. They might say: "Even if it is a draft spec, it is more of a step forward than than re-implementing the previous spec".
  2. "The hacker": ignore the proposed changes in the short term and just get the old functionality working again (in perhaps an unsanctioned way / repository to keep babel pure). "I'm bound to the previous spec and will rework my decorators to the new spec when I'm forced to"
  3. "The pragmatist": port your code from decorators to another representation in JavaScript until things get resolved. "It was fun while it lasted! I don't have that many places where I use decorators so I'll quickly unroll them, add comments for the future, and will use decorator syntax again later when this is resolved"

So to triage, it is best to do 1), 2), or 3)? Who is the best person to provide guidance on which path to take? Please weigh in!

--------------------------PS--------------------------

FYI: you can get rid of the warning in this thread by changing the following: https://github.com/babel/babel/blob/master/packages/babel-types/src/definitions/experimental.js#L23

defineType("Decorator", {
  visitor: ["expression"],
  fields: {
    expression: {
      validate: assertNodeType("Expression")
    }
  }
});

to

defineType("Decorator", {
  visitor: ["expression"],
  aliases: ["Expression"],
  fields: {
    expression: {
      validate: assertNodeType("Expression")
    }
  }
});

but it just pushes the problem to the incomplete implementation and I don't think it is part of a correct solution because the assertion happens due to the following lines not yet doing what they need to do to implement the basic class decorator tests:

not yet implementing the AST re-write correctly. Really, all of the test need to be ported and the correct AST updates implemented. We might be able to use the [[ https://github.com/babel/babel/tree/5.x/packages/babel/test/fixtures/transformation/es7.decorators/class | actual and expected output ]] from babel 5 as a guideline for what to generate, but even that is probably more of a guidelines then a strict port assuming babel 6 probably made some significant changes so it requires a re-write instead of a port to match the output (TBC).


Comment originally made by @kmalakoff on 2015-11-25T06:55:29.000Z

Just heard from @thejameskyle...https://twitter.com/thejameskyle/status/669372950848471040

@kmalakoff @sebmck @babeljs our plan is just to wait until @wycats updates his proposal.

So it looks like this is not in Needs Triage, but On Hold pending the updated proposal.

If you want to continue using decorators until this is resolved, probably the best path is to stick with Babel 5.


Comment originally made by @loganfsmyth on 2015-11-29T20:49:44.000Z

I'm going to close this since, as you've said, decorators are still pending, and their re-implementation is already being tracked in https://phabricator.babeljs.io/T2645.

Collaborator

babel-bot commented Sep 7, 2016

Comment originally made by @kmalakoff on 2015-11-23T13:54:25.000Z

--------------------------INVESTIGATIONS--------------------------

I've taken a look at the code and stepped through the tests after hoping that the comment "Decorators are not supported yet in 6.x pending proposal update." meant that decorators had been implemented in Babel 6, but were disabled due to proposal compliance reasons.

It looks like the feature was only partially implemented (you'll see it looks like it was started and then stopped mid-implementation) and only a small subset of the tests were transferred from Babel 5 (and then the tests were deleted). For example:

I believe there is probably one to three days of work to get the old implementation of decorators working with all of the [[ https://github.com/babel/babel/tree/5.x/packages/babel/test/fixtures/transformation/es7.decorators | previous tests ]].

To port the tests, it will require a bit of a steep learning curve into babel internals because decorators need to re-write a few very different scenarios of syntax (class vs property decorators) which can include switching the order of expressions in the AST, injecting new expressions, etc. After stepping through the code, I think the current, partially-implemented version may only give an indication of the final solution, but may require a bit of a rework to get all of the scenarios working.

--------------------------TRIAGE DECISION--------------------------

I don't know what the proposed spec changes are, but I believe this feature is not being actively developed pending the proposal (TBC) so we should agree the triage path while the spec gets worked out. Some potential options:

  1. "The purist": investigate the proposed spec changes and update the tests to match the latest spec and complete the port. They might say: "Even if it is a draft spec, it is more of a step forward than than re-implementing the previous spec".
  2. "The hacker": ignore the proposed changes in the short term and just get the old functionality working again (in perhaps an unsanctioned way / repository to keep babel pure). "I'm bound to the previous spec and will rework my decorators to the new spec when I'm forced to"
  3. "The pragmatist": port your code from decorators to another representation in JavaScript until things get resolved. "It was fun while it lasted! I don't have that many places where I use decorators so I'll quickly unroll them, add comments for the future, and will use decorator syntax again later when this is resolved"

So to triage, it is best to do 1), 2), or 3)? Who is the best person to provide guidance on which path to take? Please weigh in!

--------------------------PS--------------------------

FYI: you can get rid of the warning in this thread by changing the following: https://github.com/babel/babel/blob/master/packages/babel-types/src/definitions/experimental.js#L23

defineType("Decorator", {
  visitor: ["expression"],
  fields: {
    expression: {
      validate: assertNodeType("Expression")
    }
  }
});

to

defineType("Decorator", {
  visitor: ["expression"],
  aliases: ["Expression"],
  fields: {
    expression: {
      validate: assertNodeType("Expression")
    }
  }
});

but it just pushes the problem to the incomplete implementation and I don't think it is part of a correct solution because the assertion happens due to the following lines not yet doing what they need to do to implement the basic class decorator tests:

not yet implementing the AST re-write correctly. Really, all of the test need to be ported and the correct AST updates implemented. We might be able to use the [[ https://github.com/babel/babel/tree/5.x/packages/babel/test/fixtures/transformation/es7.decorators/class | actual and expected output ]] from babel 5 as a guideline for what to generate, but even that is probably more of a guidelines then a strict port assuming babel 6 probably made some significant changes so it requires a re-write instead of a port to match the output (TBC).


Comment originally made by @kmalakoff on 2015-11-25T06:55:29.000Z

Just heard from @thejameskyle...https://twitter.com/thejameskyle/status/669372950848471040

@kmalakoff @sebmck @babeljs our plan is just to wait until @wycats updates his proposal.

So it looks like this is not in Needs Triage, but On Hold pending the updated proposal.

If you want to continue using decorators until this is resolved, probably the best path is to stick with Babel 5.


Comment originally made by @loganfsmyth on 2015-11-29T20:49:44.000Z

I'm going to close this since, as you've said, decorators are still pending, and their re-implementation is already being tracked in https://phabricator.babeljs.io/T2645.

@babel-bot babel-bot closed this Sep 7, 2016

@lock lock bot added the outdated label May 7, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.