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

feat(plugin-react): add decorators option #5050

Closed
wants to merge 1 commit into from

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Sep 23, 2021

Using decorator syntax is common enough to warrant an easier way to enable it.

Before:

react({
  babel: {
    parserOpts: { plugins: ['decorators-legacy'] }
  }
})

…or by using https://babeljs.io/docs/en/babel-plugin-syntax-decorators

After:

react({
  decorators: { legacy: true },
  // or
  decorators: { beforeExport: true || false },
})

Using decorator syntax is common enough to warrant an easier way to enable it.

**Before:**

```js
react({
  babel: {
    parserOpts: { plugins: [decorator-legacy] }
  }
})
```

…or by using https://babeljs.io/docs/en/babel-plugin-syntax-decorators

**After:**

```js
react({
  decorators: { legacy: true },
  // or
  decorators: { beforeExport: true || false },
})
```
@aleclarson aleclarson added plugin: react p2-nice-to-have Not breaking anything but nice to have (priority) labels Sep 23, 2021
@haoqunjiang
Copy link
Member

haoqunjiang commented Sep 23, 2021

I don't think "common" usage is enough to justify a shorthand option.

dangerouslySetInnerHTML is also commonly used, but it was assigned a lengthy name in purpose.
IMO, decorators should be treated like that, too.

@aleclarson
Copy link
Member Author

aleclarson commented Sep 23, 2021

Hi @sodatea

dangerouslySetInnerHTML is also commonly used, but it was assigned a lengthy name in purpose.
IMO, decorators should be treated like that, too.

Hmm, this argument isn't very convincing IMO. ^^ Could you explain the negative outcomes you foresee?


Esbuild supports decorators with no configuration, so only a parser plugin is needed for decorators to work with plugin-react. It's better if we don't burden users with this information, and instead apply it for them.

In fact, I wonder if enabling decorator syntax by default would cause any issues? If not, we should do that.

Side note: I just learned that Esbuild implements TypeScript's decorator syntax (source), so the decorators option could actually just be a boolean, and we would use { legacy: true } behind the scenes. The docs for babel-plugin-transform-typescript mention this about decorators:
image

edit: Here are the alleged differences between Babel's legacy decorators and TypeScript's decorators:

  • In Babel legacy decorators, they may receive an initializer property instead of a value property, but in TypeScript there is never an initializer property, only a value property.

  • In TypeScript, decorators must return a descriptor for them to be chained with other decorators on the same class element.

  • In TypeScript, the previous point (decorator returning a descriptor) results in the class field (if decorator was used on a field) having [[Set]] semantics when useDefineForClassFields is set to false (otherwise many decorators would break).

  • In Babel, if a legacy decorator returns a descriptor, this causes the class field to always use [[Define]] semantics, regardless of the value of the loose option for the plugin-proposal-class-properties option (which seems like a bug), which is opposite of TypeScript behavior.

  • In TypeScript property decorators don't receive a descriptor, but they do in Babel.

@aleclarson
Copy link
Member Author

Since Esbuild supports TypeScript decorators by default, I suggest we enable decorators-legacy parser plugin by default, but only if @babel/plugin-proposal-decorators was not provided via Babel config (which should only be the case if the user is trying to workaround the differences between Babel legacy decorators and TypeScript decorators).

@haoqunjiang
Copy link
Member

Could you explain the negative outcomes you foresee?

Largely the same reason that esbuild only supports TypeScript decorator but not the JavaScript decorator syntax.

  • The word "legacy" in the name already tells a lot. It's a syntax that is destined to be abandoned. I think supporting such a syntax doesn't align with Vite's goal of adopting modern/standard JavaScript. Not to say that even the legacy syntax has different implementations in Babel/TypeScript, which leads to even more confusion.
  • TypeScript is not JavaScript. Ultimately, we can expect the TypeScript team to continue supporting this syntax, not ship drastic changes suddenly, and provide a reasonable path for future transition to the new syntax. It is also widely dependent on (e.g. in Angular). So it's practical to support decorators in TypeScript projects.

@aleclarson
Copy link
Member Author

aleclarson commented Sep 23, 2021

So I've opened #5057 for automatic Babel support for decorators when experimentalDecorators: true exists in the nearest tsconfig.json file (with extends support). I think that's a reasonable solution for React TypeScript projects, at least.

  • The word "legacy" in the name already tells a lot. It's a syntax that is destined to be abandoned. I think supporting such a syntax doesn't align with Vite's goal of adopting modern/standard JavaScript. Not to say that even the legacy syntax has different implementations in Babel/TypeScript, which leads to even more confusion.

Okay, due to the differences between Babel legacy decorators and TypeScript decorators, I think we should just recommend using Babel to transform decorators (with this plugin), instead of only adding the Babel parser plugin. This will avoid any confusion.

@aleclarson aleclarson closed this Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants