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

Support Babel 7 #92

Closed
gpeal opened this issue Nov 21, 2017 · 17 comments
Closed

Support Babel 7 #92

gpeal opened this issue Nov 21, 2017 · 17 comments

Comments

@gpeal
Copy link

gpeal commented Nov 21, 2017

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
Metro bundler currently has a strict dependency on babel 6. This is problematic because metro uses it both to babel-register itself which seems like a strange thing to do for a library but it also uses babel to package our app code.

We're trying to integrate TypeScript and require babel 7 but getting it to play nice with metro bundler has proven to be a challenge.

@simonbuchan
Copy link

The other thing required for TS would be customizing the module resolution extensions: is that possible ATM?

@ds300
Copy link

ds300 commented Nov 22, 2017

@simonbuchan yep that's supported with the --sourceExts option, see https://github.com/ds300/react-native-typescript-transformer for example.

@gpeal
Copy link
Author

gpeal commented Nov 27, 2017

@rafeca @arcanis Any update here? This is a blocker for TypeScript adoption at Airbnb.

@rafeca
Copy link
Contributor

rafeca commented Nov 29, 2017

Hey @gpeal,

We definitely want to upgrade to babel 7 soon, but the path to do the upgrade may not be trivial (we haven't investigated it yet, but at FB we have a few internal tools that would need to get updated).

In the meantime, can you describe the issues that you're facing when trying to use babel 7 with the current metro? (I guess that you've tried to use babel 7 from a custom transformer). I can try to assist you with that

@janicduplessis
Copy link
Contributor

janicduplessis commented Nov 30, 2017

I played with babel 7 maybe a month ago and this was also the main issue I hit. If we update babel/register to @7 it might just work but will probably break babel 6.

The only solution I see that could support both babel 6 and 7 is to ship a prebuilt version of metro and stop using babel/register.

Edit: Actually metro already ships compiled, but RN's local-cli uses babel-register.

@simonbuchan
Copy link

You could try with babel-core@bridge which forwards to @babel/core, if you can override the version used by a package with, for example, yarn resolutions, but I'm not sure if the babel 6 transforms that will be loaded by babel-register are completely compatible - so far I've not seen problems when I've used third-party transforms written for 6.

Does really seem weird that local-cli would use register: if it's for user config files, maybe it could use https://github.com/js-cli/js-interpret like everybody else?

@ds300
Copy link

ds300 commented Nov 30, 2017

There is another issue at the moment, which is that Babel is unable to have both the flow plugin and the typescript plugin loaded at the same time, due to grammatical ambiguity. Seems that it's being worked on, but for now the peeps at Artsy have figured a way around it

@gpeal
Copy link
Author

gpeal commented Dec 2, 2017

I'll update this with findings as I find them:
inputSourceMap no longer accepts a null value
but metro can set it to null here. Adding || undefined fixes this.

@gpeal
Copy link
Author

gpeal commented Dec 2, 2017

Many babel 6 plugins also don't "just work" with babel 7. I'm hitting this while compiling RCTLog.js

@Titozzz
Copy link

Titozzz commented Mar 20, 2018

Hello any updates on this ?

@simonbuchan
Copy link

https://www.npmjs.com/package/metro-babylon7 exists, and metro now has a dependency on both babel-* and @babel/*, so one assumes progress is being made - but last I checked react-native didn't build under babylon@7 so probably still a lot to go.

@rafeca
Copy link
Contributor

rafeca commented Mar 20, 2018

@simonbuchan indeed @qfox has been working really hard during the last weeks on putting all the pieces together to make metro work with babel@7.

So proper support is going to be added very soon, we'll update the task then 😄

@hzoo
Copy link

hzoo commented Mar 21, 2018

Yeah I'm not sure what metro needs to do. If y'all need to do a call to discuss this we can. I also started https://github.com/babel/babel-upgrade but it's only for basic config and doesn't handle any changes with js via codemods yet.

@pvdz
Copy link

pvdz commented Apr 3, 2018

The problem is not so much Metro itself as the complexity of the (RN) transform plugin stack. On top of that the result has to work in complex internal and external systems, without significant perf and/or size regressions.

I think I've managed to fix most of the things we ran into by now. For example, the commonjs transform plugin is now leaving "function statements" behind which will fail on Android in strict mode (es5 disallows them while es6 allows them).

While Metro should be fine with the actual transform pipeline at this point, using what it receives, there are still some places that pull it in manually. @rafeca is planning to remove or refactor those soon. After this it shouldn't matter what version of Babel you use (for as far as it already isn't a problem, anyways).

The main problem is that the AST gets passed around and the AST is strictly speaking incompatible between Babel versions. The real problem is that this isn't always the case and Babel seems to have no systems to detect cross version pollution here. This leads to very difficult to debug problems.

@hzoo are there any plans to improve the architecture in this regard? The upgrade would have been a looot easier for me if Babel could report a plugin that was assuming the wrong Babel version, or an when Babel 7 received an AST that was produced by Babel 6 somehow. There's currently no built-in way of detecting this, or of seeing a list of plugins that are running. I understand why in certain cases. My suggestion would be to request plugins to start passing on a string to identify themselves and their version spec, somehow.

@pvdz
Copy link

pvdz commented Apr 3, 2018

(@hzoo note that this architecture won't help us out at this point, although perhaps from Babel 7 to 8 later ... ;) but it will most likely help other consumers of Babel out when upgrading more complex stacks.)

@hzoo
Copy link

hzoo commented Apr 4, 2018

The main problem is that the AST gets passed around and the AST is strictly speaking incompatible between Babel versions. The real problem is that this isn't always the case and Babel seems to have no systems to detect cross version pollution here. This leads to very difficult to debug problems.

@hzoo are there any plans to improve the architecture in this regard? The upgrade would have been a looot easier for me if Babel could report a plugin that was assuming the wrong Babel version, or an when Babel 7 received an AST that was produced by Babel 6 somehow.

FYI We have made a few changes regarding this and it's definitely not an issue specific to metro. Definetely free feel to ping us or suggest what could be better on our side to make tranisition and upgrading easier, we aren't closed to such things (I mean this in general, not for this specific issue)

babel/babel#7450 adds a package that exposes a api.assertVersion(7); type thing that we also added to all the packages, should be useful for any plugin. cc @loganfsmyth

@rafeca
Copy link
Contributor

rafeca commented Apr 19, 2018

We released metro v0.33.0 yesterday, which uses babel 7. I'm going to close this issue 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants