electrode-io / electrode-native

A platform to ease integration&delivery of React Native apps in existing mobile applications
https://native.electrode.io
Other
726 stars 113 forks source link

Ambiguous module resolution - Getting started with Electrode Native guide #531

Closed LucaBlackDragon closed 6 years ago

LucaBlackDragon commented 6 years ago

Context

Issue reproduced on two different machines with Windows 10 and different versions of node (8.9.4 and 7.7.0).

electrode-native version: 1.0.12 (logged as 0.12.0 in ern --help) react-native version: 0.52.2

Steps to Reproduce

  1. Follow the steps 1), 2) and 3) from the Getting started with Electrode Native guide

Expected Behavior

Upon executing the ern run-android command (step 3) of the guide) the app should start.

Current Behavior

The app doesn't start. An Ambiguous module resolution error is shown in the console output, preceded by a very long list of errors the like of:

jest-haste-map: @providesModule naming collision:
  Duplicate module name: AnimatedNode
  Paths: C:\Users\MyUserFolder\AppData\Local\Temp\tmp-44096ovDHcTkl5Iwq\node_modules\movie-list-miniapp\node_modules\react-native\Libraries\Animated\src\nodes\AnimatedNode.js collides with C:\Users\MyUserFolder\AppData\Local\Temp\tmp-44096ovDHcTkl5Iwq\node_modules\react-native\Libraries\Animated\src\nodes\AnimatedNode.js

This warning is caused by a @providesModule declaration with the same name across two different files.

Possible Cause

It seems that Electrode (or possibly React Native) is copying the Miniapp folder inside node_modules (take a look at the paths logged in the error sample above). Due to this, there are two separate copies of React Native inside node_modules (one at root level and one inside the Miniapp folder) and this seems to cause a naming collision.

A similar case is documented in this React Native issue, in which the author accidentally included a React Native app inside the node_modules folder himself, and got the same error.

belemaire commented 6 years ago

@LucaBlackDragon Thanks for reporting this issue. This kind of error usually happens (in the Electrode Native context) when there is two different versions of React Native being used (let's say by two different MiniApps), and then a bundle is getting generated out of these. Having different versions of JS libraries is not an issue in JS, except for react-native and react which are using the haste module resolution mecanism and do not allow for different versions of RN to be in the same JS bundle.

It seems to be what is happening here as you have two different directories containing the react-native library (tmp-44096ovDHcTkl5Iwq\node_modules\movie-list-miniapp\node_modules\react-native and tmp-44096ovDHcTkl5Iwq\node_modules\react-native).

This is not that easy to check as the Miniapp is getting bundled from a temporary directory which gets deleted after the command completes (one way would be to use the create-container with the --js flag and the --out option to generate the composite in a specific directory so that you can check its content).

Is this problem happening straight from running of the 3 steps of the getting started guide or have you been doing anything else ? Because this shouldn't happen if just using ern create-miniapp followed by ern run-android as its only a single MiniApp and thus only a single version of react-native in any case.

I am pretty sure to have tested 0.12.0 of Electrode Native on Windows 10 with run-android before release, but just to make sure this is not a regression introduced in Electrode Native 0.12, could you switch back to 0.11 and retry the steps (ern platform use 0.11.3)

LucaBlackDragon commented 6 years ago

@belemaire I can confirm I have been able to reproduce the issue on a clean install of Electrode Native 0.12 and strictly following the getting started guide. I have also checked inside the temp folder and the two copies of React Native inside tmp-44096ovDHcTkl5Iwq\node_modules\movie-list-miniapp\node_modules\react-native and tmp-44096ovDHcTkl5Iwq\node_modules\react-native are the same version (0.52.2) which I suppose is expected.

I'll try to switch back to 0.11 and let you know if something changes! Thanks

LucaBlackDragon commented 6 years ago

Switching back to 0.11 I get the error described in issue #360 (see the last comment):

C:\Studio
λ mkdir ElectrodeNativeTutorial2 && cd ElectrodeNativeTutorial2

C:\Studio\ElectrodeNativeTutorial2
λ ern create-miniapp MovieListMiniApp
 ___ _        _               _       _  _      _   _
| __| |___ __| |_ _ _ ___  __| |___  | \| |__ _| |_(_)_ _____
| _|| / -_) _|  _| '_/ _ \/ _` / -_) | .` / _` |  _| \ V / -_)
|___|_\___\__|\__|_| \___/\__,_\___| |_|\_\__,_|\__|_|\_/\___|
[v0.11.3] [Cauldron: -NONE-]

√ Validity checks have passed
? Type NPM package name to use for this MiniApp. Press Enter to use the default. movie-list-miniapp
√ Validity checks have passed
Creating MovieListMiniApp MiniApp
√ Retrieving react-native version from Manifest
√ Creating MovieListMiniApp project using react-native v0.51.0. This might take a while.
MovieListMiniApp MiniApp was successfully created !
================================================
To run your MiniApp on Android :
    > cd MovieListMiniApp
followed by :
    > ern run-android
To run your MiniApp on iOS :
    > cd MovieListMiniApp
followed by :
    > ern run-ios
================================================

C:\Studio\ElectrodeNativeTutorial2
λ cd MovieListMiniApp\

C:\Studio\ElectrodeNativeTutorial2\MovieListMiniApp
λ ern run-android
 ___ _        _               _       _  _      _   _
| __| |___ __| |_ _ _ ___  __| |___  | \| |__ _| |_(_)_ _____
| _|| / -_) _|  _| '_/ _ \/ _` / -_) | .` / _` |  _| \ V / -_)
|___|_\___\__|\__|_| \___/\__,_\___| |_|\_\__,_|\__|_|\_/\___|
[v0.11.3] [Cauldron: -NONE-]

√ Starting React Native packager
- Generating Container locallyrunLocalContainerGen failed: TypeError: Cannot read property '1' of null
× Cannot read property '1' of null
An error occurred: Cannot read property '1' of null

I am trying to debug the issue in my own forked copy of the repo but had no luck so far (node setup-dev is failing in the fetching packages step of yarn install, and this is even stranger because I use yarn all day long!)... It's starting to look like ern doesn't like running on Windows 10! 😄

(for the record, I also tried to switch further back to 0.10.4 and got another error)

Jeopardye commented 6 years ago

I'm running into this exact issue on my Windows 10 computer as well now in our project.

julianoabrs commented 6 years ago

Same thing here. Stuck in this error since yesterday

pluservice commented 6 years ago

Same here too !

belemaire commented 6 years ago

Thanks for reporting this issue. Seems to impact Windows only. Antother Windows related issue was reported https://github.com/electrode-io/electrode-native/issues/552. Will look into both today, expect a patch version release (0.12.2) later today. Will ping back on this thread once done.

belemaire commented 6 years ago

The root cause of the problem has been identified. It is due to the yarn CLI version upgrade we did in Electrode Native 0.12.0 (https://github.com/electrode-io/electrode-native/pull/460) (from ^0.27.5 to ^1.3.2). For some reason, the behavior of this yarn version is different than the one on Mac in term of the directory structure generated. On Windows, the resulting node_modules structure looks non flat for some reason (as pointed by @LucaBlackDragon in inital comment,

there are two separate copies of React Native inside node_modules (one at root level and one inside the Miniapp folder) and this seems to cause a naming collision.

Yarn version 1.3.2 on Mac seems to behave in the same way as 0.27.5 in our context, but on Windows it doesn't. That being said we can't just roll back to version 0.27.5 as it creates a different issue for yarn add usage (for example yarn add C:\Dev\NodePackage doesn't work in 0.27.5 (the local path has to be prefixed by file: this is not needed in 1.3.2).

Therefore we'd like to remain on 1.3.2 and not go back to the previous version. Will investigate a bit more on why the directory structure is not as expected and try to solve this properly.

Running our system tests and unit tests on Windows is something we are unfortunately not doing at the moment. If that would have been the case the issue would have been noticed sooner and not introduce in a release. Travis is not supporting Windows, so that's a bummer. Anyway, I will make sure that we run our test suites on a Windows 10 machine (even if manually) before a release, to avoid such issues in the future as it seems our Windows user adoption is growing.

Still looking at the problem, will issue a patch release of Electrode Native as soon as possible.

belemaire commented 6 years ago

After more investigation, it was not directly due to the new version of yarn, but rather by a side effect of not properly handling windows based file system package paths. This is a bit of a mixup, but in the end it was due to this if statement not being entered https://github.com/electrode-io/electrode-native/blob/master/ern-core/src/YarnCli.js#L41 which was resulting in the double install of react-native. The problem has been identified and reported a while back in Yarn CLI (https://github.com/yarnpkg/yarn/issues/1334) but is still not fixed so we are doing a workaround in the if statement. Unfortunately this if statement was never entered in 0.12.0 due to dropping using file: prefix for file based package path even on Windows.

We are still looking in another reported issue related to scoped packages on Windows, so we won't immediately release a new patch version (as soon as we figure out this one we will issue a patch release. most probably before this week-end).

================

Meanwhile, while waiting for the next Electrode Native release, if you need this fix, you can patch the code locally with it, which will solve the issue.

The issue affects both 0.12.0 and 0.12.1 of Electrode Native. Depending on the version you are using, the path to the source file will be different.

For example, for version 0.12.0 of Electrode Native on my workstation, the path to the file to patch is :

C:\Users\benoit\.ern\versions\0.12.0\node_modules\ern-core\dist\PackagePath.js

Just replace the user name with yours in the path, as well as the version of Electrode Native you are running (either 0.12.0 or 0.12.1) and in this file, replace the following :

var FILE_PATH_RE = new RegExp(`${FILE_PATH_WITH_PREFIX_RE.source}|${FILE_PATH_WITHOUT_PREFIX_RE.source}`);

with :

var FILE_PATH_WITHOUT_PREFIX_WINDOWS_RE = /^[a-zA-Z]:\\[\\\S|*\S]?.*$/;
var FILE_PATH_RE = new RegExp(`${FILE_PATH_WITH_PREFIX_RE.source}|${FILE_PATH_WITHOUT_PREFIX_RE.source}|${FILE_PATH_WITHOUT_PREFIX_WINDOWS_RE.source}`);
LucaBlackDragon commented 6 years ago

@belemaire I can confirm you the patch is working as expected and I have been able to launch the app. Thanks for the support!

julianoabrs commented 6 years ago

@belemaire it's working here too, thanks a lot

belemaire commented 6 years ago

FYI, just released Electrode Native 0.12.2 which includes the fix to this issue.