Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[WIP] [Toolchain] Replace TS+Babel6 pipeline with just Babel 7 #991

Closed
wants to merge 4 commits into from

Conversation

alloy
Copy link
Contributor

@alloy alloy commented Mar 3, 2018

Supersedes #780.

Decided to have another go at it and go through @jamesreggio’s excellent blog post on the topic.

Our setup is a little different, because we use TypeScript. These are the things that I had to do differently:

  • Replaced @babel/preset-flow with @babel/preset-typescript
  • Removed single place we use namespace by moving to new file and re-exporting, because @babel/preset-typescript does not support namespace.
  • Patched trailing comma in node_modules/react-native-scrollable-tab-view/SceneComponent.js
  • Added step to strip all of node_modules from Flow typings, because only one of the Flow or TS Babel plugin can be used at a time. For this to work I had to patch the following files to add the @flow comment so it would properly be detected and stripped:
    • node_modules/react-native/Libraries/Experimental/SwipeableRow/SwipeableListViewDataSource.js
    • node_modules/react-native/Libraries/Interaction/PanResponder.js
    • node_modules/react-native-sentry/lib/NativeClient.js
    • node_modules/react-native-scrollable-tab-view/StaticContainer.js

Status

Well, there’s good news and there’s bad news; shall we start with the good news?

Does work

For a view that does not use Relay (i.e. only the ‘consignment flow’), this now works! 🎉

simulator screen shot - iphone 6 - 2018-03-03 at 15 08 19

Does not work

While I don’t know yet if and how it’s related to Relay, there’s definitely a correlation. I guess it’s related to some polyfilling not being right because there’s a difference between running on JSCore vs Chrome debugger and multiple versions of the same class existing (Blob).

Without JS debugger

Fails with a an exception about a require implementation missing:

simulator screen shot - iphone 6 - 2018-03-03 at 14 32 44

With JS debugger

(i.e. JS is evaluated in Chrome)

Fails with an exception about the response of the (Relay) network request not being a ‘blob’. It definitely is, but it appears we’ve got ourselves a classic ‘multiple versions of the same class’ problem, because the instance has a different prototype than the one that is being tested against:

screen shot 2018-03-03 at 14 41 10

Babel 7’s TS plugin does not support it, but out linter also doesn’t
so good riddance altogether.
Only Flow or TS Babel plugin can be enabled, so need to
preprocess the dependency sources to not have Flow types.
"prestorybook": "rnstl",
"prepare": "patch-package"
"postinstall": "yarn -s patch",
Copy link
Contributor Author

@alloy alloy Mar 3, 2018

Choose a reason for hiding this comment

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

TODO: Need to figure out the exact order of these scripts, because when trying to run the patch script from prepare the patch:packages step would fail to apply unless the patch:remove-flow-from-packages script was removed from the patch script.

@alloy
Copy link
Contributor Author

alloy commented Mar 3, 2018

Oof, gonna remove the Danger comment for now, as it’s not helpful at this very moment.

@artsy artsy deleted a comment from ArtsyOpenSource Mar 3, 2018
@jamesreggio
Copy link

Hey @alloy, a couple thoughts:

  • Could you clarify why the Flow and TypeScript presets cannot be used simultaneously? The Flow preset contains a single transform to strip the type annotations, so I'm surprised that it would conflict with the TypeScript preset. Nevertheless, in lieu of your package script, you could explicitly add plugin-transform-flow-strip-types and configure it to requireDirective.

  • Regarding the _require2 is not a function error, this is nothing more than a hunch, but... it seems to be failing on a require call that is not at the outer scope of the module. If I had to guess — given the position of the call and the name ending in 2 — one of the Babel transforms is creating an implicit, function-local variable for that require, which never gets assigned and is thus leading to that error. For these types of issues, it really helps to look directly at the transformed code. I'm probably telling you something you already know, but you can use the Web Inspector in Safari to connect to your simulator's JSContext for debugging. Given that the bug doesn't repro in remote Chrome debugging, Safari's remote debugger will be your best bet for tracking this down.

  • The problem with Blob in the Chrome debugger is reminiscent of some problems I've had with RN's polyfilling system. The packaged bundle is supposed to require the InitializeCore module first, which forcibly polyfills certain globals (including fetch), but in my experience, this doesn't always happen. (I never investigated as to why, since the workaround is so simple.) My guess is that Relay (or some other code that touches the network) is being evaluated before the polyfills are installed, and that code is holding a reference to the native fetch or XHR functions (which returns a native Blob object, which fails to match the polyfilled Blob). Without running the code myself, it's difficult to say why you're executing into the polyfill if the fetch payload (via XHR) is using non-polyfilled objects... but regardless, the solution I've found for this class of issues is to add the following import at the very top of your bundle's entry-point file (e.g., index.ios.js):

import 'InitializeCore';

Hopefully this helps to get you a little closer. Let me know how it goes.

@orta
Copy link
Contributor

orta commented Mar 3, 2018

Related to Blob, I also had a similar problem with making POSTs using XMLHttpRequests

@alloy
Copy link
Contributor Author

alloy commented Mar 5, 2018

@orta Did you record details about the fetch problem you had somewhere?

@orta
Copy link
Contributor

orta commented Mar 5, 2018

I was seeing exactly the issue that was linked below it ( jhen0409/react-native-debugger#38 ) so considered that enough documentation

@alloy
Copy link
Contributor Author

alloy commented Mar 6, 2018

@jamesreggio Thanks for taking the time! I’ll respond inline:

  • Could you clarify why the Flow and TypeScript presets cannot be used simultaneously?

    The two plugins simply can’t coexist as seen in the babylon source. I forgot the specific details, but it’s probably related to it being ambiguous to the parser how to parse code in the absence of hints such as file extensions.

    Nevertheless, in lieu of your package script, you could explicitly add plugin-transform-flow-strip-types and configure it to requireDirective.

    Alas not, this would still enable the flow plugin and thus lead to the same issue.

  • Regarding the _require2 is not a function error, this is nothing more than a hunch, but... it seems to be failing on a require call that is not at the outer scope of the module. If I had to guess — given the position of the call and the name ending in 2 — one of the Babel transforms is creating an implicit, function-local variable for that require, which never gets assigned and is thus leading to that error. For these types of issues, it really helps to look directly at the transformed code.

    Indeed, I’ll have to comb through the compiled code. Just decided to shelf that for now, because it could be a matter of a few minutes or open a rabbit hole, for which I won’t have time this sprint :/

  • The problem with Blob in the Chrome debugger is reminiscent of some problems I've had with RN's polyfilling system. […] the solution I've found for this class of issues is to add the following InitializeCore import at the very top of your bundle's entry-point file.

    Alas that did not fix the issue, but thanks for the pointer to that module anyways; it should hopefully provide me with a good starting point when I sit down to comb through the compiled code.

I’ll report back once I’ve had the time to work on this again.

@jamesreggio
Copy link

Thanks for trying those out, @alloy. Too bad none of it worked 😕

Good luck, and please keep me posted!

@alloy
Copy link
Contributor Author

alloy commented Mar 13, 2018

Re the Blob issue with a debugger connected: jhen0409/react-native-debugger#209

To elaborate a bit, this is caused by doing this:

global.XMLHttpRequest = global.originalXMLHttpRequest

@jamesreggio
Copy link

Ah, gotcha. So it's just down to the _require2 issue? Fingers crossed that that's easy to resolve.

@alloy
Copy link
Contributor Author

alloy commented Mar 13, 2018

@jamesreggio Looks like it! I’d love to spend some time on getting network debugging to work again as well, of course, but at least it doesn’t appear to be due to the changes in this PR.

@orta
Copy link
Contributor

orta commented Mar 29, 2018

Could we make babel plugin that uses flow only in files for node_modules/react-native but typescript everywhere else?

@fbartho
Copy link

fbartho commented May 23, 2018

Our jest automated tests are busted in our React-Native (ReactXP) project because of this type of issue. I'd love to hear if anybody figures out how to resolve it!

@orta
Copy link
Contributor

orta commented Jun 26, 2018

I updated us to React Native 0.56rc which moves RN to use babel 7 - #1117

@orta
Copy link
Contributor

orta commented Jul 1, 2018

This also could handle some of the tricky integration issues around flow + TS - https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages#how-babel-v7-helps

@orta
Copy link
Contributor

orta commented Oct 31, 2018

deprecated by #1210

@orta
Copy link
Contributor

orta commented Mar 12, 2019

#1210 will probably get merge in a few weeks now, so I'm closing this.

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

Successfully merging this pull request may close these issues.

4 participants