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

Legacy decorators used with private fields result in a stack trace error #10144

Open
jhpratt opened this issue Jun 29, 2019 · 12 comments

Comments

Projects
None yet
3 participants
@jhpratt
Copy link

commented Jun 29, 2019

Originally asked on Stack Overflow) by myself. If I'm missing something obvious, feel free to answer over there as well and I'll happily upvote/accept.

Bug Report

Current Behavior
The provided input and configuration leads to the following error:

TypeError: Property value expected type of string but got null
    at Object.validate (./node_modules/@babel/types/lib/definitions/utils.js:161:13)
    at validate (./node_modules/@babel/types/lib/validators/validate.js:17:9)
    at builder (./node_modules/@babel/types/lib/builders/builder.js:46:27)
    at Object.StringLiteral (./node_modules/@babel/types/lib/builders/generated/index.js:335:31)
    at ./node_modules/@babel/plugin-proposal-decorators/lib/transformer-legacy.js:93:83
    at Array.reduce (<anonymous>)
    at applyTargetDecorators (./node_modules/@babel/plugin-proposal-decorators/lib/transformer-legacy.js:84:32)
    at applyMethodDecorators (./node_modules/@babel/plugin-proposal-decorators/lib/transformer-legacy.js:70:10)
    at PluginPass.ClassExpression (./node_modules/@babel/plugin-proposal-decorators/lib/transformer-legacy.js:156:94)
    at newFn (./node_modules/@babel/traverse/lib/visitors.js:193:21)

Input Code

class Foo {
  @Decorator
  #bar = '';
}

Expected behavior/code
Successful transpilation.

Babel Configuration (.babelrc, package.json, cli command)

If proposal-decorators is swapped for syntax-decorators, Babel complains that the decorator syntax is not even enabled.

module.exports = {
  plugins: [
    ['@babel/proposal-decorators', { legacy: true }],
    ['@babel/proposal-class-properties', { loose: true }],
  ],
};

Environment

  • Babel version(s): 7.4.4
  • Node/npm version: Node 12
  • OS: Ubuntu 19.04
  • Monorepo: no
  • How you are using Babel: CLI
@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2019

Hey @jhpratt! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community
that typically always has someone willing to help. You can sign-up here
for an invite.

@jhpratt

This comment has been minimized.

Copy link
Author

commented Jun 29, 2019

Somewhat off-topic for this, but is there any way I could triage these myself? I've filed quite a few issues recently. I'm fairly certain the answer is no, but it's worth asking.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

The old decorators proposal didn't specify any interaction with class private properties.
In loose mode, they could probably be handled similar to how public loose fields are decorated, but it will require a big refactoring of our legacy decorators plugin.
In the meantime, I'd like to see there a human-friendly error message.

@jhpratt

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

@nicolo-ribaudo Surely it should be possible to parse the syntax though? I'm unable to even get that to work. My specific use case doesn't actually need the transformation, as I'm handling that myself.

@jhpratt

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

As an example using @babel/plugin-syntax-decorators in spec mode (not legacy) along with the private field transformation, in that order, I receive the following error:

SyntaxError: ./example.js: Decorators are not enabled.
If you are using ["@babel/plugin-proposal-decorators", { "legacy": true }], make sure it comes *before* "@babel/plugin-proposal-class-properties" and enable loose mode, like so:
	["@babel/plugin-proposal-decorators", { "legacy": true }]
	["@babel/plugin-proposal-class-properties", { "loose": true }]
  1 | class Foo {
> 2 |   @Decorator
    |   ^
  3 |   #bar = '';
  4 | }
  5 | 
    at File.buildCodeFrameError (./node_modules/@babel/core/lib/transformation/file/file.js:261:12)
    at NodePath.buildCodeFrameError (./node_modules/@babel/traverse/lib/path/index.js:157:21)
    at verifyUsedFeatures (./node_modules/@babel/helper-create-class-features-plugin/lib/features.js:40:18)
    at PluginPass.Class (./node_modules/@babel/helper-create-class-features-plugin/lib/index.js:82:44)
    at newFn (./node_modules/@babel/traverse/lib/visitors.js:193:21)
    at NodePath._call (./node_modules/@babel/traverse/lib/path/context.js:53:20)
    at NodePath.call (./node_modules/@babel/traverse/lib/path/context.js:40:17)
    at NodePath.visit (./node_modules/@babel/traverse/lib/path/context.js:88:12)
    at TraversalContext.visitQueue (./node_modules/@babel/traverse/lib/context.js:118:16)
    at TraversalContext.visitMultiple (./node_modules/@babel/traverse/lib/context.js:85:17)

I'm not sure if the true error is somewhere else, but this error by itself makes no sense.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

You need to transform the decorators before that the class fields plugin attemps to do so: it can't transform the private field if it is decorated otherwise.
The class fields plugin visits the ClassDeclaration node, so you'll need to visit the same node or one upper in the AST tree.

@jhpratt

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

Ah, that's an issue with my example then. I am actually visiting the ClassDeclaration node, and before the private field transform.

Here's a pastebin of the plugin I'm using. It's bundled into a single file, but I'm really just providing this for a test case — it works on its own.

Using that plugin in AST explorer, we get the following output (after formatting):

class Video extends HTMLElement {
    #refs = [];
    #video_url = null;

    get video_url() {
        return this.#video_url;
    }

    set video_url(value) {
        this.#video_url = value;
        this.#refs[0].setAttribute('src', this.video_url || '');
    }

    constructor() {
        super();
        this.attachShadow({ mode: 'open' });
        this.setAttribute('role', 'region');
        const _frag = document.createDocumentFragment();
        const _link = _frag.appendChild(document.createElement('link'));
        _link.setAttribute('rel', 'stylesheet');
        _link.setAttribute('href', './css/components/youtube-video.css');
        const _iframe = _frag.appendChild(document.createElement('iframe'));
        this.#refs.push(_iframe);
        _iframe.setAttribute('title', 'video');
        _iframe.setAttribute('src', this.video_url || '');
        this.shadowRoot.appendChild(_frag);
    }
}
window.customElements.register('x-video', Video);

There's no decorators to be found, as expected.

However, when using the same plugin in the following config, I still receive an error regarding the decorators.

module.exports = {
  plugins: [
    '@babel/syntax-jsx',
    ['@babel/syntax-decorators', { decoratorsBeforeExport: true }],
    './plugin-from-pastebin.js',
    ['@babel/proposal-class-properties', { loose: true }],
  ],
};

The error I receive is basically identical to the basic example provided above.

SyntaxError: ./Video.jsx: Decorators are not enabled.
If you are using ["@babel/plugin-proposal-decorators", { "legacy": true }], make sure it comes *before* "@babel/plugin-proposal-class-properties" and enable loose mode, like so:
        ["@babel/plugin-proposal-decorators", { "legacy": true }]
        ["@babel/plugin-proposal-class-properties", { "loose": true }]
   6 |   </>;
   7 | 
>  8 |   @Derive(get, set)
     |   ^
   9 |   #video_url = null;
  10 | }
  11 | 
    at File.buildCodeFrameError (./node_modules/@babel/core/lib/transformation/file/file.js:261:12)
    at NodePath.buildCodeFrameError (./node_modules/@babel/traverse/lib/path/index.js:157:21)
    at verifyUsedFeatures (./node_modules/@babel/helper-create-class-features-plugin/lib/features.js:40:18)
    at PluginPass.Class (./node_modules/@babel/helper-create-class-features-plugin/lib/index.js:82:44)
    at newFn (./node_modules/@babel/traverse/lib/visitors.js:193:21)
    at NodePath._call (./node_modules/@babel/traverse/lib/path/context.js:53:20)
    at NodePath.call (./node_modules/@babel/traverse/lib/path/context.js:40:17)
    at NodePath.visit (./node_modules/@babel/traverse/lib/path/context.js:88:12)
    at TraversalContext.visitQueue (./node_modules/@babel/traverse/lib/context.js:118:16)
    at TraversalContext.visitSingle (./node_modules/@babel/traverse/lib/context.js:90:19)

I don't believe this should be happening, as the decorators don't even exist by the time the proposal-class-properties plugin is reached.

Running the output from AST explorer through the same configuration above provides the expected transpiled output. Running them together does not.

If this is the correct behavior, I certainly don't understand it currently. I can try and dig through this plugin, which is where any error might be, if wanted.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

I took a look at your plugin, and I think I found

  1. a bug in your plugin and
  2. a bug in Babel which hides the bug in your plugin

  1. You are (correctly) removing class decorators (using all_decorators.filter(...).forEach(...remove())), but you are not removing element decorators (e.g. @Derive)
  2. For some reason, when doing path.toString(), Babel doesn't print the remaining decorators, making it seem like they have been removed.
@jhpratt

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

Ironically enough, I noticed that exact behavior, and assumed JavaScript was doing some weird "moved reference" thing...even though I've never seen anything like that before.

To sum up what I've gotten out of this thread:

  • Yes, I have a bug in my plugin.
  • That bug is hidden by Babel by another bug.
  • It may be worthwhile to allow parsing, but not transforming, legacy decorators with private fields enabled. It would likely need a large rewrite, so there'd definitely need to be more interest (for lack of a better word) from others before doing so. At the least, a user-friendly error should be put in place.

I'll fix the bug on my end of course. Should a new issue be filed for the bug you've found? It's hardly related to the original issue discovered when I filed this one.

Thanks for taking a look, of course!

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

I just tried to fix the bug in your plugin, I tried to parse decorators as "legacy" and it still works. Could you check?
If it does, then the only issue is that when using the plugin to transform legacy decorators and using private properties leads to that cryptic error message instead of a nicer one (this wouldn't be too hard to fix).

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

I have opened #10149 for the other bug.

@jhpratt

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

I can confirm it seems to work as expected after fixing that bug. It occurs in a couple other places in my code, so if I run into any other errors after fixing that I'll report back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.