Skip to content

Treat dynamic imports with rejection handlers as optional dependencies#1697

Closed
robhogan wants to merge 1 commit intomainfrom
export-D102145525
Closed

Treat dynamic imports with rejection handlers as optional dependencies#1697
robhogan wants to merge 1 commit intomainfrom
export-D102145525

Conversation

@robhogan
Copy link
Copy Markdown
Contributor

Summary:
Fixes: #1681

Extends the optional-dependency heuristic in collectDependencies to recognise dynamic imports that attach a rejection handler — either .catch(handler) or .then(_, handler) — anywhere in an unbroken promise chain rooted at the import() call.

Today, transformer.allowOptionalDependencies only treats require() / await import() as optional when wrapped in a try block. Library code that relies on a chained catch, e.g.

import('node:diagnostics_channel')
  .then(dc => { ... })
  .catch(() => { ... });

(seen in lru-cache for instance: https://unpkg.com/lru-cache@11.3.5/dist/esm/diagnostics-channel.js ) still fails the build with an unresolvable-module error, even though the developer has clearly opted into a runtime fallback.

This makes a pragmatic extension to collectDependencies to walk up the chain from the import() call and treat the dependency as optional if any chained call provides a rejection handler. The walk stops as soon as the chain is broken, keeping the heuristic local to the import.

Changelog:

 - **[Fix]**: Treat `import().catch()`, etc. as optional under `resolver.allowOptionalDependencies`

Reviewed By: huntie

Differential Revision: D102145525

Summary:
Fixes: #1681

Extends the optional-dependency heuristic in `collectDependencies` to recognise dynamic imports that attach a rejection handler — either `.catch(handler)` or `.then(_, handler)` — anywhere in an unbroken promise chain rooted at the `import()` call.

Today, `transformer.allowOptionalDependencies` only treats `require()` / `await import()` as optional when wrapped in a `try` block. Library code that relies on a chained `catch`, e.g.

```js
import('node:diagnostics_channel')
  .then(dc => { ... })
  .catch(() => { ... });
```

(seen in `lru-cache` for instance: https://unpkg.com/lru-cache@11.3.5/dist/esm/diagnostics-channel.js ) still fails the build with an unresolvable-module error, even though the developer has clearly opted into a runtime fallback.

This makes a pragmatic extension to `collectDependencies` to walk up the chain from the `import()` call and treat the dependency as optional if any chained call provides a rejection handler. The walk stops as soon as the chain is broken, keeping the heuristic local to the import.

Changelog:
```
 - **[Fix]**: Treat `import().catch()`, etc. as optional under `resolver.allowOptionalDependencies`
```

Reviewed By: huntie

Differential Revision: D102145525
@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync Bot commented Apr 23, 2026

@robhogan has exported this pull request. If you are a Meta employee, you can view the originating Diff in D102145525.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 23, 2026
@meta-codesync meta-codesync Bot closed this in 23e4214 Apr 23, 2026
@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync Bot commented Apr 23, 2026

This pull request has been merged in 23e4214.

GijsWeterings pushed a commit that referenced this pull request Apr 29, 2026
* Update prettier-plugin-hermes-parser in fbsource to 0.35.0

Summary:
X-link: facebook/react-native#56444

Bump prettier-plugin-hermes-parser to 0.35.0.

Changelog: [internal]

Reviewed By: SamChou19815

Differential Revision: D100850992

fbshipit-source-id: 3bb9386c007e036262b3d4fc92438e3913a12baa

* Remove unused `DependencyAnalysisPlugin.#rootDir`

Summary:
This was copied over from another plugin but is completely unreferenced and isn't relevant to dependency extraction.

Changelog: [Internal]

Reviewed By: huntie

Differential Revision: D98728740

fbshipit-source-id: c737e932007d6a1b1a2e36877d0e60cbec442c33

* Extract generic FileDataPlugin from DependencyPlugin to use as the basis for other plugins

Summary:
Refactor `DependencyPlugin` to extend a new reusable `FileDataPlugin` containing most of the unused boilerplate / default implementations.

We'll use this for a `package.json` plugin.

Changelog: Internal

Reviewed By: huntie

Differential Revision: D100990729

fbshipit-source-id: a1f5fbee10df4f3cf8506e0b7154523e4466e80d

* Replace chalk with Node core util.styleText (#1690)

Summary:
All supported Node versions in `engines` ship `util.styleText` with the array-format API, removing the need for the chalk dependency.

Pull Request resolved: #1690

Reviewed By: huntie

Differential Revision: D101224668

Pulled By: robhogan

fbshipit-source-id: 02f873fb624aab54f3b237b157024287bddaf080

* Performance: Interleave resolution attempts with building node_modules candidate paths (#1680)

Summary:
The current implementation in `resolve.js` seems slightly optimised for readability over performance, but is a hot-path that is highly impactful for bundling performance. This is slightly amplified depending on the depth of folders.

This branch aims to:
- eliminate redundant `fileSystem.lookup` and instead resolve immediately and return the earliest result
- avoid redundant array allocations and instead iterate target paths directly
- delay building a full array of target `nodeModulesPaths` when a `FailedToResolveNameError` is constructed

While this interleaves the actual resolution, avoiding redundant work and allocations makes up for this.

A very primitive (LLM-authored) benchmark can be found here, which shows an up to 10% benefit or neutral results for resolution: kitten@a36d558

Changelog: [Performance] Refactor performance sensitive metro-resolver Node module resolution hot path

Pull Request resolved: #1680

Test Plan: - CI tests should pass unchanged

Reviewed By: huntie

Differential Revision: D100149182

Pulled By: robhogan

fbshipit-source-id: a0c4533a1111cacc8363549cdffea705149bb78f

* Deploy 0.310.0 to xplat

Summary:
X-link: facebook/react-native#56543

[changelog](https://github.com/facebook/flow/blob/main/Changelog.md)
Changelog: [Internal]

Reviewed By: panagosg7

Differential Revision: D101742377

fbshipit-source-id: 57321dad493b7d17677cf9741d320e42f5ab182d

* Resolve `[metro-watchFolders]` URL prefix in bundle and entry point paths (#1695)

Summary:
Pull Request resolved: #1695

Extends Metro `Server.js` to handle `[metro-watchFolders]/N/...` prefixed entry file paths in `.bundle` and `.map` requests. This convention is already used for source file serving (powering React Native DevTools), and this change extends it to bundle resolution.

**Implementation**

Adds `_resolveWatchFolderPrefix()`, which parses `[metro-watchFolders]/N/relative/path` URLs and resolves them against the corresponding `watchFolders[N]` entry from the Metro config. Also handles `[metro-project]/...` as a prefix for the project root.

This method is called from two sites:
- `_resolveRelativePath()` — used for resolving module paths in bundle/map requests
- `_getEntryPointAbsolutePath()` — used for resolving the entry file to an absolute path

**Motivation**

The primary use case is environments where the entry point resolves to a path outside the Metro server root (e.g. via a symlink to a different filesystem mount). In these cases, `path.relative(serverRoot, entryPath)` produces a broken `../../...` path. A client (such as Expo CLI) can instead construct a `[metro-watchFolders]/N/...` URL referencing the watchFolder that contains the entry file, allowing Metro to resolve it correctly.

We have an open PR in Expo CLI that aims to use this configuration path: expo/expo#45010.

Changelog: [Internal]

Reviewed By: robhogan

Differential Revision: D102004228

fbshipit-source-id: 617d68af43846168dcabcecfa16f60d6b4bf6771

* Treat dynamic imports with rejection handlers as optional dependencies (#1697)

Summary:
Pull Request resolved: #1697

Fixes: #1681

Extends the optional-dependency heuristic in `collectDependencies` to recognise dynamic imports that attach a rejection handler — either `.catch(handler)` or `.then(_, handler)` — anywhere in an unbroken promise chain rooted at the `import()` call.

Today, `transformer.allowOptionalDependencies` only treats `require()` / `await import()` as optional when wrapped in a `try` block. Library code that relies on a chained `catch`, e.g.

```js
import('node:diagnostics_channel')
  .then(dc => { ... })
  .catch(() => { ... });
```

(seen in `lru-cache` for instance: https://unpkg.com/lru-cache@11.3.5/dist/esm/diagnostics-channel.js ) still fails the build with an unresolvable-module error, even though the developer has clearly opted into a runtime fallback.

This makes a pragmatic extension to `collectDependencies` to walk up the chain from the `import()` call and treat the dependency as optional if any chained call provides a rejection handler. The walk stops as soon as the chain is broken, keeping the heuristic local to the import.

Changelog:
```
 - **[Fix]**: Treat `import().catch()`, etc. as optional under `resolver.allowOptionalDependencies`
```

Reviewed By: huntie

Differential Revision: D102145525

fbshipit-source-id: 4241ba24eb234e7aa42bbaa133b992477ef2327c

* Deploy 0.311.0 to xplat

Summary:
X-link: facebook/react-native#56589

[changelog](https://github.com/facebook/flow/blob/main/Changelog.md)
Changelog: [Internal]

Reviewed By: bherila, marcoww6, christophpurrer

Differential Revision: D102240969

fbshipit-source-id: 09d0a17cbe135eb9e6bb8e06ea86ee67ab32a73a

* Upgrade lodash/lodash-es to 4.18.1 (CVE-2026-4800)

Summary:
Upgrade transitive dependency lodash from 4.17.21/4.17.23 to 4.18.1 and lodash-es
from 4.17.21 to 4.18.1 to remediate CVE-2026-4800 (Improper Control of Generation
of Code / Code Injection).

Updated lodash/lodash-es entries in 3 yarn.lock files:
- xplat/js/tools/react-fox/yarn.lock (lodash 4.17.21 → 4.18.1)
- xplat/js/tools/react-fox/apps/playground/yarn.lock (lodash 4.17.23 → 4.18.1)
- xplat/js/tools/metro/website/yarn.lock (lodash-es 4.17.21 → 4.18.1)

No package.json changes needed.

Reviewed By: Bellardia

Differential Revision: D102241929

fbshipit-source-id: b9a4d3ff16b2e74ea115d7954e6eebc3c0514b34

* fix(metro): Fix regression to allow scale assets to be resolved again for single asset requests (#1694)

Summary:
Resolves #1667

The prior change checks the existence of the file path too soon, and ignores that scales may be resolved later in this function.

> [!NOTE]
> Unrelated to this change, the last change feels a bit ad-hoc and incomplete. We should replace the `fs.promises.readdir` call with the file map. That's out of scope for this PR though.

Changelog: [Fix] Fix regression to allow single asset request to resolve scaled assets

Pull Request resolved: #1694

Test Plan: - Unit tests added

Reviewed By: huntie

Differential Revision: D102142213

Pulled By: robhogan

fbshipit-source-id: 778f142fb4d1d07ce11d8b4661b8bcc87b6b5150

---------

Co-authored-by: Marco Wang <marcoww@meta.com>
Co-authored-by: Stian Jensen <me@stianj.com>
Co-authored-by: Phil Pluckthun <phil@kitten.sh>
Co-authored-by: Mike Vitousek <mvitousek@meta.com>
Co-authored-by: Alex Hunt <huntie@meta.com>
Co-authored-by: George Zahariev <gkz@meta.com>
Co-authored-by: Sandeep Kudterkar <kudterkasandeep@meta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolver should not fail on dynamic (optional) imports that cannot be found?

1 participant