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

npm module should be precompiled to ES5 #7850

Closed
fab1an opened this issue May 31, 2016 · 21 comments
Closed

npm module should be precompiled to ES5 #7850

fab1an opened this issue May 31, 2016 · 21 comments
Labels
Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.

Comments

@fab1an
Copy link

fab1an commented May 31, 2016

The JS-code that react-native exports in package.json main should be es5, precompiled by babel.

There a lot of issues popping up right now, because all code and libraries that depends on react-native need to know how to compile react-native as well, which is difficult to get right, and changes over time.

One such bug: vitalets/react-native-extended-stylesheet#10

It also breaks code with custom .babelrc.

I have already started trying to precompile it and documented in this issue, which I will close: #7666

Here is the branch with my preliminary work: https://github.com/fab1an/react-native/tree/precompile-babel.

@javache
Copy link
Member

javache commented Jun 3, 2016

cc @davidaurelio

@fab1an
Copy link
Author

fab1an commented Jun 11, 2016

@fab1an
Copy link
Author

fab1an commented Jun 11, 2016

Progress so far on master:

  1. copied the whole Libaries folder and cleaned out any non-js files:

    cp -R Libraries/ src/
    find src -type f -not -name "*js" -print0 | xargs -0 rm
    
  2. created a .babelrc in src and installed the presets and plugins:

    {
      "presets": ["es2015", "react"],
      "plugins": [
        "transform-object-rest-spread",
        "transform-class-properties",
        "syntax-trailing-function-commas",
        "babel-plugin-syntax-async-functions"
      ]
    }
  3. compiled everything from src back into Libraries

    node_modules/.bin/babel src/ -d Libraries

    Warning appearing is:

    You or one of the Babel plugins you are using are using Flow declarations as bindings.
    Support for this will be removed in version 6.8. To find out the caller, grep for this
    message and change it to a `console.trace()`.
    
  4. linked this using npm link into an existing project and tried running the packager:
    (./node_modules/react-native/packager/packager.sh start --reset-cache)

    This gives me an error about files being duplicated:

    [12:47:16 PM] <END>   Building (deprecated) Asset Map (68ms)
    Failed to build DependencyGraph: @providesModule naming collision:
    Duplicate module name: NavigationTreeNode
    Paths: /Users/fabian/dev/forks/react-native/Libraries/CustomComponents/Navigator/Navigation/NavigationTreeNode.js 
    collides with /Users/fabian/dev/forks/react-native/src/CustomComponents/Navigator/Navigation/NavigationTreeNode.js
    

For testing purposes I just moved the src folder (the original source) elsewhere and ran the packager again.

Now I don't get warnings by the packager but an Execution exception on the phone.

The error in logcat:

E/ReactNativeJS(18234): Error: Can't find variable: process, stack:
E/ReactNativeJS(18234): http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:2427:11
E/ReactNativeJS(18234): loadModuleImplementation@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:127:8
E/ReactNativeJS(18234): guardedLoadModule@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:70:32
E/ReactNativeJS(18234): _require@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:54:18
E/ReactNativeJS(18234): http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:2075:20
E/ReactNativeJS(18234): loadModuleImplementation@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:127:8
E/ReactNativeJS(18234): guardedLoadModule@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:70:32
E/ReactNativeJS(18234): _require@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:54:18
E/ReactNativeJS(18234): http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:1710:25
E/ReactNativeJS(18234): loadModuleImplementation@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:127:8
E/ReactNativeJS(18234): guardedLoadModule@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:70:32
E/ReactNativeJS(18234): _require@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:54:18
E/ReactNativeJS(18234): http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:1540:26
E/ReactNativeJS(18234): loadModuleImplementation@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:127:8
E/ReactNativeJS(18234): guardedLoadModule@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:70:32
E/ReactNativeJS(18234): _require@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:54:18
E/ReactNativeJS(18234): http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:1523:23
E/ReactNativeJS(18234): loadModuleImplementation@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:127:8
E/ReactNativeJS(18234): guardedLoadModule@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:70:32
E/ReactNativeJS(18234): _require@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:54:18
E/ReactNativeJS(18234): http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:1468:101
E/ReactNativeJS(18234): loadModuleImplementation@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:127:8
E/ReactNativeJS(18234): guardedLoadModule@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:63:37
E/ReactNativeJS(18234): _require@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:54:18
E/ReactNativeJS(18234): global code@http://localhost:8081/js/index.android.bundle?platform=android&dev=true&hot=true&minify=false:70337:9

@javache
Copy link
Member

javache commented Jun 11, 2016

In your custom package can't you extend babel-preset-react-native? For various reasons I don't think we'd want to ship a transpiled version, it also wouldn't match what we use internally, which could lead to incompatibilities.

@fab1an
Copy link
Author

fab1an commented Jun 11, 2016

I don't know. React-native is the first npm-project I know of, that relies on it's dependencies to compile it's source.

I made a styling library (https://github.com/fab1an/react-native-tachyons) which needs to call the StyleSheet constructor once, and wasn't able to make it compile. I tried installing a couple of babel-plugins, but after a while I gave up and now I'm just having the developer pass over the StyleSheet constructor at runtime. Plus I didn't want my library to break when rn's babel-settings change.

I think this coupling makes depending code extremely brittle. Just having an unrelated .babelrc lying in my home-directory can break react-native projects in confusing ways.

I frankly don't see the point of not publishing it precompiled. The packager can still compile the developer's code? My guess is it's because of node-haste which facebook uses internally?

If it's not going to be precompiled than another idea would be to

  1. use a second transformer for the developer's code which doesn't interact with react-native code and
  2. prevent react-native from picking up other .babelrc than it's own.

@satya164
Copy link
Contributor

satya164 commented Jun 11, 2016

@javache Can you outline the problems in transpiling? In my experience, not shipping a transpiled version causes lots of compatibility issues,

  • It's quite problematic to use with other code which is pre-compiled with Babel
  • Sourcemaps are screwed up when you've dependency that has a .babelrc with different config, sometimes it even breaks the code
  • It's kinda difficult to use different configs for the other code when the project has different types of code mixed, so you're pretty much locked to use RN's preset for everything
  • Since the packager transpiles node_modules, we've to be careful about which transforms we enable not to affect third party code, for example, enabling strict mode breaks many third party modules

@fab1an I don't think haste modules will cause issues here. Since we only want to transpile code, not bundle it.

cc @cpojer @davidaurelio

@ide
Copy link
Contributor

ide commented Jun 12, 2016

These are the issues with transpiling I can think of. If they are addressed I would be in support of transpiling:

  1. Alignment between Facebook and open source: The biggest issue is misalignment with Facebook's internal transpiling workflow. Facebook's codebase is written using modern JS (in practice, most of ES6 and some future proposals like class properties), and the static resources system is responsible for transpiling the code on the fly. I believe pretty strongly we should align the open-source and Facebook-internal workflows for React Native so that we are running the same code in similar environments, and share incentives.
  2. Source maps: When debugging code in a pre-transpiled package, source maps should just work. Debugging code under node_modules shouldn't be considerably more difficult or different than debugging your application code, especially because application code often gets refactored into npm packages.
  3. Robust publishing: Have a recommended way to set up package.json and .babelrc that is easy to understand and usually just works. npm has "preinstall, "postinstall, "publish", and "prepublish" scripts that can be surprising (ex: npm install runs "prepublish") so we should sort this out for everyone so they don't have to think about the publishing process.
  4. Smooth development: Aside from the unrelated issue with Watchman and symlinks, it's pretty smooth to work on an npm package. I edit the ES6+ source code and reload the app. One thing that's especially important is that the code is not stale. With pre-transpiling it can be frustrating when you're editing the source ES6 and forget to transpile it or have a syntax error and your app keeps loading old code. I would want us to address those two pitfalls.
  5. Installation from Git: Sometimes I'll fork someone's repo, send them a PR, and in the meantime upgrade my package.json to point to my fork ("react-xxx": "ide/react-xxx"). I think the only way to keep this working is to transpile on every Git commit and to commit the build artifacts to Git.

@ide
Copy link
Contributor

ide commented Jun 12, 2016

Just having an unrelated .babelrc lying in my home-directory can break react-native projects in confusing ways.

I'm in favor of fixing this. We could look into making it so that we search for .babelrc only in the directory with the package.json that depends on "react-native", for example.

@fab1an
Copy link
Author

fab1an commented Jun 12, 2016

@ide Thanks your comments.

syntax: I think many codebases nowadays too are ES6 + static props (+ async/wait). Mine are. I just sometimes use additional babel-plugins for code-generation and the like.

npm publishing: I want to point out publish-please here, which is a better replacement for npm publish and circumvents the broken prepublish.

.babelrc: Babel's default behaviour is to search for .babelrc in parent-directories, which is fine.
The problem is that when I develop and get an error, I know it has to do with my code or the compilation-settings for my code. I don't expect the error having something to do with another project being compiled "invisibly":
Did my code use react-native wrong? Did my compiler-settings break my code? Or did my compiler-settings break react-native?

I think the packager should compile everything inside react-native (not everything node_modules, there could be third party code there) with it's settings which won't pick up other .babelrcs.
The developer's code should be compiled separately with the react-native preset he can override in a .babelrc if he wishes to.

smooth development: So a main-requirement is for you to be able to work "inside" node_modules fast. Shouldn't that be possible with plain npm link and a watcher which transpiles files on the fly inside the dependency?

@satya164
Copy link
Contributor

satya164 commented Jun 12, 2016

@ide great points 👍

Alignment between Facebook and open source

Certainly a difficult problem to solve. I wonder how this works for other FB projects which are written in ES6 and published as transpiled code, e.g. - https://github.com/facebook/draft-js

Source maps

I'm not sure if packager reads .map files when present. This would solve the issues with source maps. And it wouldn't only be beneficial to React Native, but all NPM packages which publish transpiled versions along with source-maps.

Robust publishing

As per my understanding, npm prepublish only runs when publishing to npm, installing from GIT, and running npm install in the project which has it. This behaviour kinda confusing, but in practice, I haven't found it to be much problematic as long as the prepublish script only does transpiling, and nothing else.

Installation from Git

Since npm pre-publish runs when installing from Git, it should address this use case I think.


I've a crazy idea, which I think should solve most of the problems, except the source-map one. How about, we don't publish the transpiled version, but publish the source as always. When the packager starts up, and code in react-native has changed, it'll transpile it before anything else, as a separate step which uses babel-preset-react-native and doesn't read user's .babelrc file.

  • For most people, it won't matter, packager will take a bit longer to startup for the first time, but all subsequent runs will have no difference.
  • For developers who modify the source, or FB folks, they'll run the packager anyways before testing their changes, so packager can transparently transpile RN code.

We already had some discussion regarding pre-compiling, which can be useful - https://www.facebook.com/groups/reactnativeoss/permalink/1528961550733807/

@satya164 satya164 added Packager Type: Discussion Long running discussion. labels Jun 12, 2016
@satya164
Copy link
Contributor

Related - #8074

@fab1an
Copy link
Author

fab1an commented Jun 13, 2016

@satya164 that sounds like a solution, but too rnative developers start the packager everytime anew? Or how would it detect changes?

Maybe it can be solved with just separate babel-passes.

@satya164
Copy link
Contributor

@fab1an packager can maintain a cache for this I guess.

@lacker
Copy link
Contributor

lacker commented Jun 15, 2016

+1 for transpiling to ES5 - the current react native behavior goes against npm best practices. Jest requires special hacks to support this, and a lot of tools simply don't work with react native because they assume shipping es5.

@davidaurelio
Copy link
Contributor

I have an internal task for this.

@fab1an
Copy link
Author

fab1an commented Jun 21, 2016

I just spent two hours debugging another case where packaging didn't work :( : react-native picked up a .babelrc, inside a dependency (a library I wrote) in node_modules. That .babelrc wasn't used at all, since my library comes precompiled.
So another problem seems to be that the packager tries to compile third-party libraries which are already compiled?

Addendum:
I think an easy way to fix a lot of issues is whitelisting the babelrc that the transformer is allowed to read.

@fab1an
Copy link
Author

fab1an commented Jun 27, 2016

Should I open a separate issue for the .babelrc stuff? I think it can be fixed without precompiling react-native.

@lifehackett
Copy link

Really glad the source of this issue seems to have been identified. The linked to babelHelpers.typeof issue has been a huge pain point for me. Any idea when a fix might get released?

@lifehackett
Copy link

Any activity on this? This just cropped up for me again and is a PITA to get back to a working state.

@lacker
Copy link
Contributor

lacker commented Nov 29, 2016

To give an update, there have been a bunch of discussions and I think people agree that 1/ the ideal state would be shipping ES5 and 2/ it's a nontrivial amount of work to get there. The last person who vaguely promised to think about this more and hinted that he might be the right person to deal with this without concretely committing to anything was @cpojer so I'll tag him in this sentence.

@cpojer
Copy link
Contributor

cpojer commented Nov 29, 2016

Let's continue this in #10966.

@cpojer cpojer closed this as completed Nov 29, 2016
@facebook facebook locked as resolved and limited conversation to collaborators Jul 19, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.
Projects
None yet
Development

No branches or pull requests

9 participants