nrwl / nx-labs

A collection of Nx plugins
MIT License
130 stars 48 forks source link

expo: not working with pnpm isolated deps #34

Open psteinroe opened 2 years ago

psteinroe commented 2 years ago

Hey!

Thanks for your work, highly appreciated!

Unfortunately, we are using pnpm in our mono repo and it seems like the expo plugin does not work with that. We continue to get errors like

Unable to resolve module uuid/v5 from /Users/steinroe/Developer/monorepo/node_modules/.pnpm/expo@44.0.6_@babel+core@7.17.0/node_modules/expo/build/environment/getInstallationIdAsync.js: uuid/v5 could not be found within the project or in these directories:
  ../../node_modules/.pnpm/expo@44.0.6_@babel+core@7.17.0/node_modules/expo/node_modules
  ../../node_modules/.pnpm/expo@44.0.6_@babel+core@7.17.0/node_modules
  ../../node_modules/.pnpm/node_modules
  ../../node_modules

and

Unable to resolve module @babel/runtime/helpers/interopRequireDefault from /Users/steinroe/Developer/monorepo/apps/mobile-app/index.js: @babel/runtime/helpers/interopRequireDefault could not be found within the project or in these directories:
  ../../node_modules

I also tried is by cloning the example https://github.com/xiongemi/nx-expo-poetry. Using it with npm works fine, but after deleting package-lock.json and installing dependencies with pnpm, it is again unable to start. The only way it works with pnpm if I use pnpm import to import the lock file. Is there something I can do to make it work with a fresh pnpm install?

psteinroe commented 2 years ago

EDIT: Got it fixed. pnpm users need to add node-linker=hoisted to the .npmrc file (and create one if it does not exist yet). Maybe we can add a note to the docs? Or even add it into the template?

source: https://github.com/rhyek/expo-monorepo-issue/compare/pnpm...pnpm-fixed

l1qu1d commented 2 years ago

@steinroe Thanks for the fix! Also note: PNPM users also need to run pnpm i after creating .npmrc or adding node-linker=hoisted. If you have to create the file, make sure it's in your package.json file directory.

evelant commented 1 year ago

EDIT: Got it fixed. pnpm users need to add node-linker=hoisted to the .npmrc file (and create one if it does not exist yet). Maybe we can add a note to the docs? Or even add it into the template?

source: rhyek/expo-monorepo-issue@pnpm...pnpm-fixed

adding node-linker=hoisted kinda defeats the purpose of pnpm doesn't it? One of the big benefits of pnpm is that it doesn't hoist all your deps to workspace root and therefore prevents you from having phantom dependencies. To make pnpm work with expo/react-native you need to use https://microsoft.github.io/rnx-kit/docs/tools/metro-config to add symlink support to metro.

psteinroe commented 1 year ago

That's true! We ended up not adding the expo app to the monorepo because of it... do you have an example of setting up metro properly? Would love to add it back in!

evelant commented 1 year ago

All you really need to do is add rnx-kit/metro-config per the docs and react-native/expo should work with pnpm. I had to add a couple packages to .npmrc public-hoist-pattern because some react-native build is still configured with hardcoded paths that rely on sub-dependencies being flattened into node-modules. Here's what I added:

public-hoist-pattern[]=*expo*
public-hoist-pattern[]=*react-native-gradle-plugin*
public-hoist-pattern[]=*@react-native-community/cli-platform-android*
public-hoist-pattern[]=*@react-native-community/cli-platform-ios*

The only tough issue I've run into so far is that one package (@react-native-google-signin/google-signin) won't build on android because gradle gets confused by symlinks and believes that there are duplicate classes.

leggomuhgreggo commented 1 year ago

@evelant does the rnx-kit approach work with EAS builds? If so did it require any eas-build-pre-install or postinstall scripts?

Thanks!

evelant commented 1 year ago

@leggomuhgreggo Yep the rnx-kit/metro-symlinks-resolver is actually all you need, plus installing a copule of extra dev dependencies since pnpm doesn't allow undeclared deps and there are actually quite a few that are undeclared in expo (that is expo relies on them being flattened into node_modules so that they just happen to resolve by chance when using yarn/npm).

You can actually see a minimal example here: https://github.com/evelant/test-pnpm-ios-headers-path-bug

That won't build on iOS because of use_frameworks! but if you turn that off (remove the "expo-build-properties" plugin from app.json) it should be a minimal example of pnpm on an expo project with dev client built by EAS.

One other note is that I found the EAS build machines have an outdated pnpm. I had to add npm install -g pnpm to my eas-build-pre-install script to update it. Other than that it works great!

leggomuhgreggo commented 1 year ago

No kiddin! This is very exciting news. Thank you @evelant!

Update: I tried this out and got it to work! Golly, what a delightful turn of events.

I can confirm that bit about needing a later version of pnpm than 7.0.0. They actually just updated the iOS@latest image with 7.11.0, earlier today. But Android is still rocking the outdated stuff. So I think your npm install -g pnpm workaround is still required for the time being.

I may throw together an example repo for other folks who're interested -- and hopefully follow up on some of these questions:

But I feel like the rnx-kit/metro-config approach is a worthy option for the default plugin behavior.

In any case -- thanks again for shining a light on it!

Old Notes How are you managing your app-level package.json? \- Does it still require the wildcard dependency work-around? \- Are you using the nx sync-deps command? or perhaps keeping it updated manually? \- Or is it actually all handled from the monorepo-level package.json? (gonna start playing with this now -- thanks again!) EDIT: > I had to add npm install -g pnpm to my eas-build-pre-install script I remember hearing that the initial 7.0.0 version of pnpm had a couple issues that needed to be patched. Is that why you bumped it in the pre install script or was it just on general principles? EDIT2: Looks like the latest EAS build image ([updated](https://github.com/expo/expo/commit/efdfa177aa070b281694173d756e296d07c697c2) earlier today) has pnpm 7.11.0 🎉 EDIT3: I guess pnpm 7.11.0 is only available on the latest iOS image -- the latest Android infra still seems to use 7.0.0. I haven't checked with Android yet, but all my EAS build attempts that didn't specify `ios.image = "latest"` failed -- so it seems like that version bump is definitely required. (Thanks for the heads up there!)
evelant commented 1 year ago

@leggomuhgreggo I'm using this setup in a monorepo no problem. Just set up your repo per the pnpm instructions on workspaces and your packages will work. You don't need much at all in your top level package.json with pnpm other than scripts and the "pnpm": section to apply the various tools pnpm has for working around poorly defined packages.

I've actually put experimenting with nx on hold for now so I'm not sure how that will interact.

As for esbuild there's @rnx-kit/metro-serializer-esbuild but that doesn't work in development, seems it's only for production bundling. Personally I didn't want to take the risk of having different behavior in production due to using a different bundling process.

I'm actually building my app with tsc now so I can use ts+ then I just let metro bundle the output from that. Works fine with EAS, just compile with tsc in the post install step.

mwarger commented 1 year ago

No kiddin! This is very exciting news. Thank you @evelant!

Update: I tried this out and got it to work! Golly, what a delightful turn of events.

I can confirm that bit about needing a later version of pnpm than 7.0.0. They actually just updated the iOS@latest image with 7.11.0, earlier today. But Android is still rocking the outdated stuff. So I think your npm install -g pnpm workaround is still required for the time being.

I may throw together an example repo for other folks who're interested -- and hopefully follow up on some of these questions:

  • Check whether sync-deps + app-level package.json are still required
  • Confirm transient native dependencies
  • Figure out workspace asset resolution / libraries
  • Investigate EAS dependency caching
  • Explore esbuild + build size / tree shaking

But I feel like the rnx-kit/metro-config approach is a worthy option for the default plugin behavior.

In any case -- thanks again for shining a light on it!

Old Notes

@leggomuhgreggo did you ever have a chance to put together a minimal repo of this?

goughjo02 commented 1 year ago

@evelant ... this is kind of off-topic but since you mentioned it, is @react-native-google-signin/google-signin deprecated in favor of expo-auth-session/providers/google? or do they have different capabilities?

goughjo02 commented 1 year ago

to add to what @evelant said about using the metro config . I have tried combinging that with the symlink resolver in a fresh app (pnpm, nx, expo) and it appears to be serving fine on iphone simulator