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

Fix metro-minify-terser being resolved from the wrong folder #1172

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tido64
Copy link
Contributor

@tido64 tido64 commented Jan 10, 2024

Summary

In a pnpm-like setup, metro-minify-terser gets resolved from the wrong folder.

Test plan

n/a

@facebook-github-bot facebook-github-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 Jan 10, 2024
@tido64
Copy link
Contributor Author

tido64 commented Jan 10, 2024

cc @huntie @robhogan

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jan 10, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (702e1b8) 83.31% compared to head (b3d0b9c) 83.31%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1172   +/-   ##
=======================================
  Coverage   83.31%   83.31%           
=======================================
  Files         207      207           
  Lines       10633    10633           
  Branches     2642     2642           
=======================================
  Hits         8859     8859           
  Misses       1774     1774           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

robhogan added a commit to robhogan/metro that referenced this pull request Jan 24, 2024
…x pnpm (etc) resolution

Summary:
Related: facebook#1172

This is the minimally invasive change to fix resolution of the default `minifierPath: 'metro-minify-terser'`, especially under isolated node_modules layouts.

`minifierPath` is required/resolved only from `metro-transform-worker`:
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/utils/getMinifier.js#L22
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/index.js#L656

Per the [current docs for `minifierPath`](https://metrobundler.dev/docs/configuration/#minifierpath), a module specifier relative to `metro-transform-worker` is explicitly acceptable:

> Type: string (default: 'metro-minify-terser')
> Path, or package name resolvable from metro-transform-worker, to the minifier that minifies the code after transformation.

Unlike facebook#1172 (thanks tido64 for flagging), this doesn't modify the defaults and can be released in a patch release. The approach in that PR (using fully resolved paths in config) may be the better long-term fix though, so this patch shouldn't be regarded as superseding it.

Changelog:
```
 - **[Fix]:** Move `metro-minify-terser` dependency to fix resolution under isolated node_modules (pnpm, etc).
```

Differential Revision: D53000650
facebook-github-bot pushed a commit that referenced this pull request Jan 24, 2024
…x pnpm (etc) resolution (#1199)

Summary:
Pull Request resolved: #1199

Related: #1172

This is the minimally invasive change to fix resolution of the default `minifierPath: 'metro-minify-terser'`, especially under isolated node_modules layouts.

`minifierPath` is required/resolved only from `metro-transform-worker`:
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/utils/getMinifier.js#L22
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/index.js#L656

Per the [current docs for `minifierPath`](https://metrobundler.dev/docs/configuration/#minifierpath), a module specifier relative to `metro-transform-worker` is explicitly acceptable:

> Type: string (default: 'metro-minify-terser')
> Path, or package name resolvable from metro-transform-worker, to the minifier that minifies the code after transformation.

Unlike #1172 (thanks tido64 for flagging), this doesn't modify the defaults and can be released in a patch release. The approach in that PR (using fully resolved paths in config) may be the better long-term fix though, so this patch shouldn't be regarded as superseding it.

Changelog:
```
 - **[Fix]:** Move `metro-minify-terser` dependency to fix resolution under isolated node_modules (pnpm, etc).
```

Reviewed By: huntie

Differential Revision: D53000650

fbshipit-source-id: 251f52c17af58c88ebedb387ac92ecbe788772ea
robhogan added a commit that referenced this pull request Jan 29, 2024
…x pnpm (etc) resolution (#1199)

Summary:
Pull Request resolved: #1199

Related: #1172

This is the minimally invasive change to fix resolution of the default `minifierPath: 'metro-minify-terser'`, especially under isolated node_modules layouts.

`minifierPath` is required/resolved only from `metro-transform-worker`:
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/utils/getMinifier.js#L22
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/index.js#L656

Per the [current docs for `minifierPath`](https://metrobundler.dev/docs/configuration/#minifierpath), a module specifier relative to `metro-transform-worker` is explicitly acceptable:

> Type: string (default: 'metro-minify-terser')
> Path, or package name resolvable from metro-transform-worker, to the minifier that minifies the code after transformation.

Unlike #1172 (thanks tido64 for flagging), this doesn't modify the defaults and can be released in a patch release. The approach in that PR (using fully resolved paths in config) may be the better long-term fix though, so this patch shouldn't be regarded as superseding it.

Changelog:
```
 - **[Fix]:** Move `metro-minify-terser` dependency to fix resolution under isolated node_modules (pnpm, etc).
```

Reviewed By: huntie

Differential Revision: D53000650

fbshipit-source-id: 251f52c17af58c88ebedb387ac92ecbe788772ea
robhogan added a commit that referenced this pull request Jan 30, 2024
…x pnpm (etc) resolution (#1199)

Summary:
Pull Request resolved: #1199

Related: #1172

This is the minimally invasive change to fix resolution of the default `minifierPath: 'metro-minify-terser'`, especially under isolated node_modules layouts.

`minifierPath` is required/resolved only from `metro-transform-worker`:
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/utils/getMinifier.js#L22
 - https://github.com/facebook/metro/blob/v0.80.4/packages/metro-transform-worker/src/index.js#L656

Per the [current docs for `minifierPath`](https://metrobundler.dev/docs/configuration/#minifierpath), a module specifier relative to `metro-transform-worker` is explicitly acceptable:

> Type: string (default: 'metro-minify-terser')
> Path, or package name resolvable from metro-transform-worker, to the minifier that minifies the code after transformation.

Unlike #1172 (thanks tido64 for flagging), this doesn't modify the defaults and can be released in a patch release. The approach in that PR (using fully resolved paths in config) may be the better long-term fix though, so this patch shouldn't be regarded as superseding it.

Changelog:
```
 - **[Fix]:** Move `metro-minify-terser` dependency to fix resolution under isolated node_modules (pnpm, etc).
```

Reviewed By: huntie

Differential Revision: D53000650

fbshipit-source-id: 251f52c17af58c88ebedb387ac92ecbe788772ea
@tido64
Copy link
Contributor Author

tido64 commented Feb 5, 2024

@robhogan: Is this no longer needed?

@robhogan
Copy link
Contributor

robhogan commented Feb 5, 2024

@robhogan: Is this no longer needed?

Not urgently, since we added the dependency to metro-transform-worker in the last release - thanks for the PR though, as well as identifying the problem it kicked off a good discussion about how we should be configuring these kinds of dependencies (relative vs absolute module paths vs code injection, etc). Cache portability makes things a little tricky.

We’ll revisit this kind of config holistically in a future breaking release and might merge this PR then - it may be the best approach long term, but if you’d rather close it for now, feel free.

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants