Add configuration option to suppress require cycle warnings

[hsource]

Summary

In many projects there are require cycles in the node_modules dependencies that don't really need to be fixed and that don't seem to affect the functioning of the app. This change adds an option to let users selectively suppress them.

Unlike many of the workarounds in #287, this requires opting in per-package, so that you're never blindsided by bugs caused by require cycles in subpackages you own or maintain.

Closes #287

Test plan

1. Built metro in my own branch using yarn build && yarn lerna run prepare-release
2. Copied over changed files (require.js, getPreludeCode.js, getPrependedScripts.js) into my own node_modules
3. Changed metro.config.js and ran react-native start with the extra config below

module.exports = {
  // ...
  resolver: {
    // ...
    ignoreRequireCyclePrefixes: [
      'node_modules/react-native-firebase',
      'node_modules/react-native-maps',
      'node_modules/rn-fetch-blob',
      'node_modules/quill-delta',
    ],
  },
};

Before

 WARN  Require cycle: node_modules/rn-fetch-blob/index.js -> node_modules/rn-fetch-blob/polyfill/index.js -> node_modules/rn-fetch-blob/polyfill/Blob.js -> node_modules/rn-fetch-blob/index.js

Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle.
 WARN  Require cycle: node_modules/rn-fetch-blob/index.js -> node_modules/rn-fetch-blob/polyfill/index.js -> node_modules/rn-fetch-blob/polyfill/XMLHttpRequest.js -> node_modules/rn-fetch-blob/index.js

Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle.
 WARN  Require cycle: node_modules/rn-fetch-blob/index.js -> node_modules/rn-fetch-blob/polyfill/index.js -> node_modules/rn-fetch-blob/polyfill/Fetch.js -> node_modules/rn-fetch-blob/index.js

Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle.
 WARN  Require cycle: node_modules/react-native-firebase/dist/utils/apps.js -> node_modules/react-native-firebase/dist/modules/core/app.js -> node_modules/react-native-firebase/dist/utils/apps.js

Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle.
 WARN  Require cycle: node_modules/react-native-firebase/dist/modules/admob/index.js -> node_modules/react-native-firebase/dist/modules/admob/Interstitial.js -> node_modules/react-native-firebase/dist/modules/admob/index.js

After

No warnings

[motiz88]

@hsource this is awesome, thanks for the PR! There are some details to discuss but I love the idea of making this warning less noisy and more actionable.

1. I would go further and ignore require cycles in node_modules by default. It's a safe assumption that users don't have direct control over cycles in third-party modules, and that code that's already published to npm was at least minimally tested so the warning is a false positive there.
2. I wouldn't limit the filtering capability to prefixes. How about changing this to accept regexes? (We can call it e.g. requireCycleIgnorePatterns.) That way we can check for node_modules anywhere in the project, not just at the root.
3. Let's make sure this works out of the box for Windows paths. One way would be to use regexes as per (2), and make the default pattern check for both forward and backward slashes around node_modules.
4. I wonder if we should check all the modules in a cycle against the patterns (and suppress the warning if there are any matches?) or whether checking just the first module is enough. We can start with the current solution, though.

[motiz88]

Since this is dev-only, I would avoid injecting this variable if isDev is false.

Also, let's make sure this name starts with __METRO, e.g.

Suggested change

	`__IGNORE_REQUIRE_CYCLE_PREFIXES__=${JSON.stringify(
	ignoreRequireCyclePrefixes,
	)}`,
	`__METRO_IGNORE_REQUIRE_CYCLE_PREFIXES__=${JSON.stringify(
	ignoreRequireCyclePrefixes,
	)}`,

[hsource]

Done in the latest change!

[hsource]

@hsource this is awesome, thanks for the PR! There are some details to discuss but I love the idea of making this warning less noisy and more actionable.

1. I would go further and ignore require cycles in node_modules by default. It's a safe assumption that users don't have direct control over cycles in third-party modules, and that code that's already published to npm was at least minimally tested so the warning is a false positive there.
2. I wouldn't limit the filtering capability to prefixes. How about changing this to accept regexes? (We can call it e.g. requireCycleIgnorePatterns.) That way we can check for node_modules anywhere in the project, not just at the root.
3. Let's make sure this works out of the box for Windows paths. One way would be to use regexes as per (2), and make the default pattern check for both forward and backward slashes around node_modules.
4. I wonder if we should check all the modules in a cycle against the patterns (and suppress the warning if there are any matches?) or whether checking just the first module is enough. We can start with the current solution, though.

@motiz88 Thanks for the review and sorry for the delay. I had some spare time and just cleaned up this PR.

I applied all of your changes. The default RegExp looks a bit messy, but I've verified that it works with both slashes and backslashes. Can you take another look?

[Latropos]

@hsource Thank you for your work 💚, any update on it?

[hsource]

I think it's just awaiting a pull. @motiz88 or anyone else can take a look when they have time!

[Latropos]

@motiz88 🥺 🙏 💔❓
Could sb look at this PR?
@mroch @pieterv @robhogan

[Latropos]

@hsource bump?

[facebook-github-bot]

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robhogan]

Tests are failing because acorn@^5.1.2, which we use only as a dev dependency, is too old to support for..of syntax. I'll update that first.

Tests are failing because we verify that the transformed bundle parses as ES5 -

metro/packages/metro/src/integration_tests/execBundle.js

Line 19 in 36092e2

acorn.parse(code, {ecmaVersion: 5});

That's breaking on the for..of loop since we removed that transform from metro-react-native-babel-preset (which we use for these tests) in #792.

[facebook-github-bot]

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robhogan]

Hi @hsource - sorry for the delay with this and thanks again for the contribution. After I imported this we had some further review suggestions internally:

• Switching the config type from $ReadOnlyArray<string> to $ReadOnlyArray<RegExp> (precedent in blockList)
• Respecting __METRO_GLOBAL_PREFIX__ in the global variable name.

I made these changes over at robhogan@eeb286b but since it's a non-trivial change to the proposal I wanted to check with you before pushing to this branch.

Am I ok to pull that change in to your PR here and re-import for review?

[hsource]

Am I ok to pull that change in to your PR here and re-import for review?

@robhogan Yes! Go for it. Thanks a ton for improving on this

[facebook-github-bot]

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.