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

chore(babel-preset-expo): remove babel decorators plugin #27857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nodonisko
Copy link

Why

I removed @babel/plugin-proposal-decorators from babel-preset-expo because there it's impossible to use this preset if you want to use with other version than legacy and it's impossible to override this settings.

User that really needs support for legacy decorators will need to add one line to their babel.config.js but I think it's worth because others can use modern decorators (and there was TODO: remove comment too in code).

How

Test Plan

Testing is not necessary because it seems that Expo doesn't use legacy decorators anywhere.

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 25, 2024
@Nodonisko Nodonisko force-pushed the chore/remove-babel-decorators-plugin branch from 0782284 to 30fd459 Compare March 25, 2024 20:45
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 25, 2024
@@ -290,8 +290,6 @@ function babelPresetExpo(api: ConfigAPI, options: BabelPresetExpoOptions = {}):

plugins: [
...extraPlugins,
// TODO: Remove
[require('@babel/plugin-proposal-decorators'), { legacy: true }],
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a major breaking change to the language feature support in expo. Unclear if we want to approach the change like this.

Copy link
Author

Choose a reason for hiding this comment

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

I agree this is breaking change, but it's impossible to change version of decorators if you want to use expo preset.

I can try to explore more possible solutions. One that popped right now to my mind is to use env variables, but it's quite uncommon approach.

Copy link
Member

Choose a reason for hiding this comment

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

Overall I am supportive of removing the decorators plugin by default, especially legacy: true. Also, decorators in general are a language feature that look simpler than the amount of power they have and until the proposal reaches Stage 4 I am in favor of requiring a little friction to use decorators specifically.

However, there needs to be a smooth migration path and clear guidance for developers beyond, "Breaking change: @babel/plugin-proposal-decorators is no longer included in babel-preset-expo". Some things that could help are to:

  1. Explain how to tell whether your app relies on this plugin. Hopefully, this is easy to tell since you'd get a syntax error when bundling.
  2. Provide developers with a flag like enableLegacyDecorators that they can use for one SDK version to opt back in to the old behavior. However, if they can just add back [require('@babel/plugin-proposal-decorators'), { legacy: true }] to their own Babel configuration, I don't feel a need for a flag.
  3. Explain clearly how to add back the plugin.

It would also be good to verify that popular libraries don't need this feature. AFAIK the expo repo itself doesn't rely on this feature but if a library like react-navigation does, I would be more hesitant to change things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add plugin-proposal-decorators option to the preset options which can be set to false to disable the plugin (which can be used to add a new plugin) or be used to pass additional options to the plugin.

Copy link
Author

Choose a reason for hiding this comment

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

@EvanBacon I was not aware that preset could have options. It seems like best option. I will rewrite my code to use it.

Copy link
Author

Choose a reason for hiding this comment

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

@EvanBacon @ide I reworked PR and added decoratorsPluginVersion which could be set to specific version or false and defaults to legacy.

    // default
    decoratorsPluginVersion: 'legacy',

    // Use specific version of decorators proposal
    decoratorsPluginVersion: '2023-05',

    // Disable decorators plugin
    decoratorsPluginVersion: false,

I am not entirely sure if I like mixing of string and boolean but I noticed similar approach is used for lazyImports option. Alternatively I can add disableDecoratorsPlugin option in addition to version to avoid this.

I don't think it's necessary to cover other options because there is only one addition option decoratorsBeforeExport but that can be used only with legacy so it's basically deprecated.

@Nodonisko Nodonisko force-pushed the chore/remove-babel-decorators-plugin branch from 30fd459 to e2f0ee5 Compare March 26, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants