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

[expo] Add support for pnpm isolated modules #23937

Merged
merged 4 commits into from
Aug 13, 2023

Conversation

byCedric
Copy link
Member

@byCedric byCedric commented Aug 13, 2023

Why

This PR makes the expo autolinking scripts compatible with isolated modules (e.g. from pnpm). It's part of other PRs that make our packages compatible and more resilient in monorepo projects.

How

  • iOS: resolve expo-modules-autolinking from the __dir__
    (current directory of the file expo/scripts/autolinking.rb)
  • Android: resolve expo-modules-autolinking from expo location
    (there is no dir alternative, so we need to resolve the expo > expo-modules-autolinking chain from project root)

Test Plan

  • $ pnpm create expo ./test-isolated-modules -t tabs
  • $ cd ./test-isolated-modules
  • Patch expo with the changes from this PR
  • Both Android and iOS run commands should work:
    • $ pnpm expo run:ios
    • $ pnpm expo run:android

Or see this repo: https://github.com/byCedric/expo-pnpm-tests/blob/main/patches/expo%4049.0.6.patch

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Aug 13, 2023
@byCedric byCedric marked this pull request as ready for review August 13, 2023 17:09
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Aug 13, 2023
@byCedric
Copy link
Member Author

byCedric commented Aug 13, 2023

As a side note, we could clean up some scattered node resolution scripts in our packages by adding a helper function in expo.

The main issue is that we mostly resolve things from the projectRoot, which can't resolve nested dependencies that aren't listed as direct project dependencies. But, since expo is a direct project dependency, we could add something like a expo/resolve or expo/find helper command.

With that, we can clean up the resolutions a bit. E.g.:

// Example API
const path = require('path');

function resolve(dependencyChain: string[], dependencyFile?: string) {
  const packageFilePath = dependencyChain.reduce((currentDir, dependency) => (
    !currentDir
      ? require.resolve(path.join(dependency, 'package.json'))
      : require.resolve(path.join(dependency, 'package.json'), { paths: [currentDir] })
  ));

  if (dependencyFile) {
    return path.join(path.dirname(packageFilePath), dependencyFile);
  }
  
  return path.dirname(packageFilePath);
}

// Example - root path of dependency
require('expo/resolve')(['expo', '@expo/cli']);

// Example - native script file within dependency
require('expo/resolve')(['expo', 'expo-modules-autolinking'], 'scripts/android/autolinking_implementation.gradle')

The above code should work fine, as it will execute something similar to this:

image

@byCedric
Copy link
Member Author

Chatted with @tsapeta, will do another pass / PR with the helper function (described above) once all major issues are resolved.

@byCedric byCedric merged commit 725c2b0 into main Aug 13, 2023
7 checks passed
@byCedric byCedric deleted the @bycedric/expo/isolated-modules-support branch August 13, 2023 18:35
EvanBacon pushed a commit that referenced this pull request Aug 23, 2023
# Why

This PR makes the `expo` autolinking scripts compatible with isolated
modules (e.g. from pnpm). It's part of other PRs that make our packages
compatible and more resilient in monorepo projects.

- #23867
- #23926

# How

- **iOS:** resolve `expo-modules-autolinking` from the `__dir__`
  _(current directory of the file `expo/scripts/autolinking.rb`)_
- **Android:** resolve `expo-modules-autolinking` from `expo` location 
_(there is no __dir__ alternative, so we need to resolve the `expo` >
`expo-modules-autolinking` chain from project root)_

# Test Plan

- `$ pnpm create expo ./test-isolated-modules -t tabs`
- `$ cd ./test-isolated-modules`
- Patch expo with the changes from this PR
- Both Android and iOS run commands should work:
  - `$ pnpm expo run:ios`
  - `$ pnpm expo run:android`

Or see this repo:
https://github.com/byCedric/expo-pnpm-tests/blob/main/patches/expo%4049.0.6.patch

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants