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

Optional imports in Metro #120

Closed
brentvatne opened this issue Apr 13, 2019 · 13 comments
Closed

Optional imports in Metro #120

brentvatne opened this issue Apr 13, 2019 · 13 comments
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject

Comments

@brentvatne
Copy link
Contributor

brentvatne commented Apr 13, 2019

Introduction

A longstanding pain point with Metro is that there is no ergonomic solution for conditionally importing packages. This is becoming especially noticeable by a larger part of the community now that we are moving packages out of core with the lean-core project. If my library depends on NetInfo from react-native and a user of the library has a version of react-native where NetInfo has been removed, I should be able to import NetInfo from react-native-net-info instead:

// NetInfo.js
import { NetInfo } from 'react-native';

let _NetInfo = NetInfo;
if (!_NetInfo) {
  _NetInfo = require('react-native-net-info').default;
}

export default NetInfo;

Ideally, this should let users on old versions of react-native continue to use the version of NetInfo that is exported from react-native or the react-native-community version if that's not available.

Another way to solve this is through dependency injection, but this is not particularly ergonomic for developers.

import { setNetInfoInterface } from 'react-native-offline';
import { NetInfo } from 'react-native';
setNetInfoInterface(NetInfo);
// this same thing 10 more times for other libs you're using below

This same situation applies to things like AsyncStorage, Slider, WebView, and the many other components extracted from core.

This behavior would be useful for other cases beyond just backwards compatibility. For example, react-native-paper would like to provide easy default behavior if the user has react-native-vector-icons installed, but it should be optional to have that package (perhaps the developer chooses to provide their own icon library). In react-navigation we may want to use BlurView from the Expo SDK if inside a managed Expo project, otherwise we may want to import from react-native-blur-view.

The Core of It

If I want to perform some behavior when package X is available, otherwise fallback to Y, it can't be done without 1) a Babel plugin (which brings its own set of problems) or 2) some silly hacks.

  1. We could use https://github.com/satya164/babel-plugin-optional-require, which allows us to write the following:
let a;

try {
  a = require('optional-module');
} catch (e) {
  // Handle failure from loading the module
}

This is basically exactly what we need. There are two problems: first, users need to install and configure this for it to work, and this is not a very good pre-requisite for a library to have. Second, Metro/Babel caching is pretty aggressive here and if you loaded this code without having 'optional-module' installed and then afterwards installed it and ran this code again, it require('optional-module') would still throw an error.

  1. Libraries can set some global variable when they are loaded, so other libraries can check if they exist based off of the presence of that global. For example, we had to create global.__expo so that various libraries could detect if they are in an Expo managed project and behave slightly differently in that case. The only other option would have been that we made libraries that do this (eg: react-navigation) either not have some features or make the library exclusively work in Expo managed projects.

Discussion points

  • I believe that we may be too late at tackling this problem to prevent some headache in terms of maintaining backwards compatibility for existing extracted lean-core modules. Libraries that depend on these packages will need set compatibility to >= the version where they were extracted, or require that users install a module which may or may not work with the older version of react-native that they are using. Does anybody have some ideas for how libraries can maintain compatibility with APIs that existed in react-native core and now live in other packages?
  • If we support optional imports similar to what is described above, we can mitigate problems like this in the future. What are we losing by adding support for optional imports? Is it worth it?
  • Is there a better alternative to optional imports than the try/catch approach mentioned above? (we might want to make sure that whatever we propose here, it should also be something that can work in Webpack because of Haul and react-native-web)
@cpojer
Copy link
Member

cpojer commented Apr 14, 2019

Send a diff to Metro and I'll merge it. Make sure it is optional (I'm fine if it's by default on).

@connectdotz
Copy link

connectdotz commented Jan 21, 2020

not sure if anybody has acted on this issue, judging the try/catch block require() is still throwing errors when missing, it didn't seem to be addressed yet. (?)

@brentvatne is absolutely right that the need to have the optional-dependency will grow when more lean-core breakout occurs. I bumped into this issue when trying to integrate @react-native-community/toolbar-android... Anyway, I submitted a PR (facebook/metro#511) to address the optional-dependency in metro... @cpojer, or anybody interested, please review/comment there, hopefully, we can get this roll out soon...

facebook-github-bot pushed a commit to facebook/metro that referenced this issue Mar 11, 2020
Summary:
**Summary**

This PR added support for the common optional dependency pattern: enclose require() within try/catch block, which is already supported by webpack.

While the lack of optional dependency support is not new, due to [lean-core](facebook/react-native#23313) initiative, it has become obvious that we should just address this to provide better customer-experience. See the excellent discussion in react-native-community/discussions-and-proposals#120.

**Change Outline**
The changes are mainly in 3 areas:
1. visit try/catch block and mark optional dependencies (`collectDependencies.js`)
1. during dependency resolve, if the optional dependency fails to resolve, just ignore it. (`traverseDependencies.js`)
1. add a config (`optionalDependency`, defaults to true) to disable and customize (exclude dependency from being optional) (`metro-config`)

The rest are just tunneling through the new config option, added/modified tests.

**Test plan**

tested the new create react-native app with `react-native-vector-icons`  and optional `react-native-community/toolbar-android` (in try/catch block from `react-native-vector-icons` ) :
- without the optional dependency support, metro complained `react-native-community/toolbar-android` not found and abort
- with the optional dependency, the app starts up without any problem.

**Discussion**
- if we found `import()` in the try block, should we also consider it "optional"? The common pattern is to use `require()` but one would think `import()` should be just the same, no? This PR considered any dependency found in the try block "optional", but that can be changed easily.
- I put the new config `optionalDependency` in the config.resolver, not sure if it is best to be in resolver or transformer?
- should we add some kind of warning message for omitted optional dependency? verbose flag?
Pull Request resolved: #511

Reviewed By: rickhanlonii

Differential Revision: D20280664

Pulled By: cpojer

fbshipit-source-id: 8c4abebfbf2a2f4bfa65f64e2916c1e9182977f0
@brentvatne
Copy link
Contributor Author

brentvatne commented Apr 21, 2020

hurray, @connectdotz has landed this in facebook/metro#511 with the help of @cpojer!

@lfkwtz
Copy link

lfkwtz commented Apr 23, 2020

any documentation on where this landed? looks like the try/catch concept from what i can see. also - using this would still (likely) require that someone has upgraded RN to 0.63 where metro-react-native-babel-preset is at 0.59, correct?

@connectdotz
Copy link

connectdotz commented Apr 24, 2020

yeah, react-native needs to upgrade its dependency to pick up this change (metro 0.59). Currently RN 0.63 => react-native-community/cli 4.7 => metro 0.58.

@thymikee
Copy link
Member

Or you can use Yarn resolutions?

@gorhom
Copy link

gorhom commented Oct 28, 2020

thanks @connectdotz for working on this feature, but i have tried it on react-native v0.63.3 -> @react-native-community/cli v4.10.0 -> metro v0.58.0, but i still get metro error.

// Not working 
let test;
try {
  test = import('@gorhom/test');
} catch {
}
// Not working 
let test
try {
  test = require('@gorhom/test');
} catch {
}

@connectdotz
Copy link

Optional dependency is released in metro 0.59

@gorhom
Copy link

gorhom commented Oct 29, 2020

thanks @connectdotz

@douglasjunior
Copy link

I'm using metro 0.67.0 and React native 0.68, with these metro configs:

const { getDefaultConfig } = require('metro-config');

module.exports = (async () => {
  const {
    resolver: { sourceExts, assetExts },
  } = await getDefaultConfig();
  return {
    transformer: {
      babelTransformerPath: require.resolve('react-native-svg-transformer'),
      allowOptionalDependencies: true, // I tried to add this but it didn't work
      getTransformOptions: async () => ({
        transform: {
          experimentalImportSupport: false,
          inlineRequires: true,
        },
      }),
    },
    resolver: {
      assetExts: assetExts.filter(ext => ext !== 'svg'),
      sourceExts: [...sourceExts, 'svg'],
    },
  };
})();

And my code:

let instance;

try {
  if (require('@react-native-firebase/app')?.default?.app()) {
    const analytics = require('@react-native-firebase/analytics')?.default;
    instance = analytics();
  }
} catch (err) {
  console.warn(err);
}

The error:

Error: undefined Unable to resolve module @react-native-firebase/analytics from <my-project>/src/App.tsx: @react-native-firebase/analytics could not be found within the project or in these directories:
  node_modules
  24 | try {
  25 |   if (require('@react-native-firebase/app')?.default?.app()) {
> 26 |     const analytics = require('@react-native-firebase/analytics')?.default;
     |                                ^
  27 |     instance = analytics();
  28 |   }
  29 | } catch (err) {

Any tips?

landabaso added a commit to bitcoinerlab/descriptors that referenced this issue Sep 12, 2023
- **src/ledger.ts**:
  Updated the method of importing `ledger-bitcoin` to improve compatibility with React Native's Metro bundler. Replaced the dynamic import (`await import('ledger-bitcoin')`) with a direct `require` statement to mitigate bundler issues.

- **package.json & package-lock.json**:
  Bumped the version from 1.1.0 to 1.1.1 to reflect the improvements.

- **CHANGELOG.md**:
  Documented the changes made for version 1.1.1, highlighting the adjustment in importing `ledger-bitcoin` for React Native compatibility.

Reference for the Metro bundler issue: react-native-community/discussions-and-proposals#120
@sdwvit
Copy link

sdwvit commented Mar 18, 2024

@douglasjunior did you solve it?

@douglasjunior
Copy link

douglasjunior commented Mar 19, 2024

@douglasjunior did you solve it?

Yeah, I solved it by using a babel plugin https://github.com/satya164/babel-plugin-optional-require

@sdwvit
Copy link

sdwvit commented Mar 20, 2024

@douglasjunior thanks, I solved it too by using optionalDependenciesMeta field in package.json and just using latest Expo version /w default metro config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject
Projects
None yet
Development

No branches or pull requests

9 participants