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

Incorrect Babel transforms when running with "npm link" #10882

Closed
artdent opened this issue Nov 11, 2016 · 21 comments
Closed

Incorrect Babel transforms when running with "npm link" #10882

artdent opened this issue Nov 11, 2016 · 21 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@artdent
Copy link
Contributor

artdent commented Nov 11, 2016

Description

If you use "npm link" (or "yarn link") on a React Native component repository imported by your main repository, the wrong Babel presets are used in the component repository.

Edit: the Babel error is fixable by adding the correct devDependencies in the component, but doing so exposes other problems during packaging. See below for updated repro steps.

Reproduction

https://github.com/artdent/NpmLinkDemo demonstrates the problem. This is a stock repository created from react-native init 0.37.0, with one extra commit to import and use an arbitrary component, 'react-native-snap-carousel'.

Component repository:

git clone https://github.com/archriss/react-native-snap-carousel
cd react-native-snap-carousel
yarn link

Main repository:

git clone https://github.com/artdent/NpmLinkDemo
cd NpmLinkDemo
yarn install
yarn link react-native-snap-carousel
npm start
react-native run-ios

Expected behavior: the component attempts to render.
Actual behavior: SyntaxError /Users/jacob/triggr/react-bug-repro/react-native-snap-carousel/index.js: Unexpected token (6:21)

The SyntaxError is on the 'static' keyword, which should be permitted by react-native Babel preset. Likewise with the spread operator. If you remove those unsupported features, you get syntax errors at the places that use JSX.

I had to do two workarounds in react-native-snap-carousel to get it working as it did without 'yarn link': add a .babelrc with the react-native preset, and add react as an explicit dependency. Neither of these should be necessary. (And the latter can cause version collisions very quickly.)

I believe that this is not the same issue as #637 (initial symlink support for the packager) or #4968 (nonspecific import resolution caching problems).

Additional Information

  • React Native version: 0.37.0
  • Other versions: yarn 0.16.1, npm 3.9.5, node 6.2.2, react-native-cli 1.2.0, react 15.3.1
  • Platform: both
  • Operating System: MacOS
@hramos
Copy link
Contributor

hramos commented Nov 11, 2016

cc @mkonicek

@lacker
Copy link
Contributor

lacker commented Nov 11, 2016

I thought different Babel presets basically aren't supported, and you should be using the babel-preset-react-native everywhere.

@artdent
Copy link
Contributor Author

artdent commented Nov 11, 2016

I'm not trying to use a non-standard Babel preset. The bug is that the standard one, babel-preset-react-native, is not applied correctly to files in the repository referenced via npm link.

@ericvicenti
Copy link
Contributor

I think the babel preset should always be defined in the .babelrc. So I believe that is the expected behavior.

You should specify "react" as a peerDependency in your package.json, not a direct dependency. Does that work?

@artdent
Copy link
Contributor Author

artdent commented Nov 14, 2016

@ericvicenti Which repository's package.json are you referring to? In the repro that I've posted, the main repository (which was created with react-native init) has react as a direct dependency, and the component repository (an arbitrary React component that I do not control) does not depend on react at all. I believe that that's all correct; it would be better for react-native-snap-carousel to specify react as a peer dependency, but that doesn't affect this bug.

Similarly, the main repository has a standard .babelrc containing babel-preset-react-native, and the component repository has no babelrc. This was all specified in my original report.

@ericvicenti
Copy link
Contributor

The "an arbitrary React component that I do not control" should absolutely have react as a peerDependency, because it is a React component (even for functional components, JSX depends on React).

From my understanding, the component repository SHOULD have a babelrc that points to the rn preset.

I know you aren't the component author in this case, but maybe you can submit a PR to the repo to fix these things? Or you could fork it.

@artdent
Copy link
Contributor Author

artdent commented Nov 14, 2016

The proposed fix -- adding .babelrc, plus a devDependencies entry on babel-preset-react-native -- solves half the problem. The component now fails to load with:

Unable to resolve module react from /Users/jacob/triggr/react-bug-repro/react-native-snap-carousel/index.js: Module does not exist in the module map or in these directories:
      /Users/jacob/triggr/react-bug-repro/react-native-snap-carousel/node_modules

    This might be related to https://github.com/facebook/react-native/issues/4968
    To resolve try the following:
      1. Clear watchman watches: `watchman watch-del-all`.
      2. Delete the `node_modules` folder: `rm -rf node_modules && npm install`.
      3. Reset packager cache: `rm -fr $TMPDIR/react-*` or `npm start -- --reset-cache`."

(The workarounds proposed in that error message don't fix the problem.)

If I give react-native-snap-carousel a dev dependency on react, rather than a peer dependency, then everything renders. That's the second half of the workaround that I described in my original report.

Unfortunately, that workaround breaks for components that depend on react-native (unlike react-native-snap-carousel, but like the non-oss components that caused me to file this bug). If you add a dev dependency on react-native in the component, then you end up with errors like the following when running npm start:

Error: @providesModule naming collision:
  Duplicate module name: String.prototype.es6
  Paths: /Users/jacob/triggr/triggr_native_components/node_modules/react-native/packager/react-packager/src/Resolver/polyfills/String.prototype.es6.js collides with /Users/jacob/triggr/triggr_native_ios/node_modules/react-native/packager/react-packager/src/Resolver/polyfills/String.prototype.es6.js

This error is caused by a @providesModule declaration with the same name across two different files.
    at HasteMap._updateHasteMap (/Users/jacob/triggr/triggr_native_ios/node_modules/react-native/packager/react-packager/src/node-haste/DependencyGraph/HasteMap.js:155:13)
    at module.getName.then.name (/Users/jacob/triggr/triggr_native_ios/node_modules/react-native/packager/react-packager/src/node-haste/DependencyGraph/HasteMap.js:115:31)
    at process._tickCallback (internal/process/next_tick.js:103:7)

And that error is where my knowledge of React internals is exceeded. It looks to me like one of the following holds:

  1. the component should add devDependencies on react-native, etc., but there's a bug that causes @providesModule collisions as described above
  2. the component should not have to add devDependencies on react-native, etc., in order to be usable with npm link.

@uqix
Copy link

uqix commented Nov 21, 2016

@artdent same problem as yours, any progress?

@Exilz
Copy link

Exilz commented Nov 21, 2016

Same problem, I can't find a proper workaround that solves every case.
If you find one, I'd be happy to check your PR @artdent

@ericvicenti
Copy link
Contributor

cc @davidaurelio @ljharb

@ljharb
Copy link

ljharb commented Nov 21, 2016

@artdent every react project should have both a peerDependency and a devDependency on react.

@artdent
Copy link
Contributor Author

artdent commented Nov 21, 2016

@ljharb That's sensible. If the project also imports, say, react-native and third-party components like react-native-mixpanel, should it include those in both peerDependencies and devDependencies? (My instinct is yes: for instance, unit tests won't work in that repository unless you have dev dependencies.)

@ljharb
Copy link

ljharb commented Nov 21, 2016

Yes. In every npm project, full stop, any form of dependency that's a singleton should be a peer dep with as wide a semver range as possible; anything that's a peer dep should also be a dev dep.

artdent added a commit to artdent/react-native-easy-toast that referenced this issue Nov 22, 2016
Two changes:
- Set the dev and peer dependencies correctly. This happens to not be
  necessary for react-native-easy-toast as it is written, but it would
  be necessary if e.g. we were writing unit tests for this component.
- Add a babelrc, required for compilation when running with 'npm link'.
artdent added a commit to artdent/NpmLinkDemo that referenced this issue Nov 22, 2016
@artdent
Copy link
Contributor Author

artdent commented Nov 22, 2016

Thanks for the confirmation. That makes it pretty easy to isolate this bug. It occurs when the main project depends on 'react-native' and a component project, linked with npm link, also has a devDependency on 'react-native'. Furthermore, you have to have run npm install in the component project. (If you haven't, you hit the missing babel preset error.) The packager thus ends up finding two copies of react-native; even if they're the same version, it complains.

Here's the output of running npm start in the main package with that setup:

~/triggr/react-bug-repro/NpmLinkDemo$ npm start

> NpmLinkDemo@0.0.1 start /Users/jacob/triggr/react-bug-repro/NpmLinkDemo
> node node_modules/react-native/local-cli/cli.js start

Scanning 712 folders for symlinks in /Users/jacob/triggr/react-bug-repro/NpmLinkDemo/node_modules (6ms)
[snip startup banner]
Looking for JS files in
   /Users/jacob/triggr/react-bug-repro/NpmLinkDemo
   /Users/jacob/triggr/react-bug-repro/react-native-easy-toast

[Hot Module Replacement] Server listening on /hot

React packager ready.

jest-haste-map: @providesModule naming collision:
  Duplicate module name: String.prototype.es6
  Paths: /Users/jacob/triggr/react-bug-repro/react-native-easy-toast/node_modules/react-native/packager/react-packager/src/Resolver/polyfills/String.prototype.es6.js collides with /Users/jacob/triggr/react-bug-repro/NpmLinkDemo/node_modules/react-native/packager/react-packager/src/Resolver/polyfills/String.prototype.es6.js

This warning is caused by a @providesModule declaration with the same name across two different files.
[snip hundreds of similar warnings]

The component repository that demonstrates this is https://github.com/artdent/react-native-easy-toast. It's a fork of an arbitrary react-native component with one extra commit to fix up its deps in a manner that exposes the bug. I've also pushed an extra commit to the main project, https://github.com/artdent/NpmLinkDemo, to use the new component and trigger the bug.

@artdent
Copy link
Contributor Author

artdent commented Nov 29, 2016

For those also encountering this bug:

  • I think the babel issue, which has a workaround, might be fixed by c83fc07 in v.39.0-rc.0.
  • rm -r node_modules/react-native in the component repository is a workaround for the @providesModule naming collision bug, at the cost of e.g. not being able to run tests until you reinstall the react-native module.

For the react-native maintainers:

  • The @providesModule errors are fixed if I change packager/react-packager/src/Resolver.js to replace roots: opts.projectRoots, with roots: opts.projectRoots.slice(0, 1), when instantiating DependencyGraph. But I have no idea if that's actually an appropriate change or if it would have other consequences.

@Exilz
Copy link

Exilz commented Dec 12, 2016

Any update on this issue ? I used to work directly in node_modules (horrible I know, but at least I could work).
But now, because of this issue #11301 i cannot even work anymore.

🆘

@JulianKingman
Copy link

npm link doesn't work yet, pwaiting for this pull request: #9009

@Exilz
Copy link

Exilz commented Mar 23, 2017

How do people even work on modules since then ?? I don't understand how this issue is still there

@javiercr
Copy link
Contributor

@Exilz I think most of us are making changes to our librares under node_modules and then copy and pasting to the proper folder...

@Exilz
Copy link

Exilz commented Mar 23, 2017

@javiercr that's what I thought...

PSA : Don't ever yarn while doing this, because contrary to npm install, it will remove folders that are not in your package.json, erasing your current work.

Believe me, it hurts.

@hramos
Copy link
Contributor

hramos commented Jul 20, 2017

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

  • Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
  • If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
  • Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.

@hramos hramos added the Icebox label Jul 20, 2017
@hramos hramos closed this as completed Jul 20, 2017
@facebook facebook locked as resolved and limited conversation to collaborators Jul 20, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 20, 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.
Projects
None yet
Development

No branches or pull requests

10 participants