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-config): Replace source-map snapshots for metro-transformer-worker with mappings tests #25872

Merged
merged 13 commits into from
Dec 14, 2023

Conversation

kitten
Copy link
Member

@kitten kitten commented Dec 12, 2023

Why

  • Closes ENG-10873
  • This reverts the lock on @babel/generator@~7.20.5 to @babel/generator@^7.20.5 to allow upgrades past 7.21.0
  • @babel/generator@7.21.0 shipped a fix that added names of “friendly call frames” to its source maps (feat: Generate sourcemaps of friendly call frames babel/babel#15022). This caused the source maps to change to include internal names for in-scope identifiers such as globalThis and _interopRequireDefault
    • This was a completely backwards-compatible change for us, but this was obscured by source map output being a little hard to read

When applied, this PR replaces our source map snapshots with tests that check exact identifiers in the input and output against their mappings and names.
This should make it easier to identify changes to the source maps and allow us to debug them more easily in the future.

A note has been added to any assertions in the tests that break when downgrading below 7.21.0, but for now, I haven't added any additional tests that account for this, since I'm not expecting us to downgrade to the lower version again, and to instead keep all @babel/* packages in @expo/metro-config upgraded in lockstep.

How

  • All source-map snapshots for metro-transformer-worker have been deleted
  • @jridgewell/trace-mapping has been added to parse the worker’s source maps (with a small converter function)
    • This is an alternative to the ubiquitous source-map package, and it’s also used in Babel. It avoids parsing source maps in wasm.
    • When adding source-map instead, we’d have to work around our virtual FS mock
  • All source-map snapshots have been replaced with checks for both input and output identifiers against the actual source-maps
  • Additional tests have been added to check for internal globals, which previously caused the source maps to break

Test Plan

Note: When testing this, placing the pinned version back to @babel/generator@~7.20.5 causes some name properties to be missing in the source-map output for injected globals, as expected.

Checklist

Copy link

linear bot commented Dec 12, 2023

Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarn watch and yarn build produce different outputs in @expo/metro-config to support lazy loading. We could probably get rid of this in a future PR, but this is why the build results are all different. I'll open a ticket to remove this extra lazy loading.

@EvanBacon
Copy link
Contributor

Be sure to add the changelog entry

@kitten kitten force-pushed the @kitten/metro/fix-sourcemap-transform-tests branch from 3ef8fdc to 0690a97 Compare December 13, 2023 12:46
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Dec 13, 2023
@kitten kitten force-pushed the @kitten/metro/fix-sourcemap-transform-tests branch from 0690a97 to 2784326 Compare December 13, 2023 23:32
@kitten kitten force-pushed the @kitten/metro/fix-sourcemap-transform-tests branch 2 times, most recently from 52a5e3a to d358da5 Compare December 14, 2023 00:07
@kitten kitten merged commit 5a3e924 into main Dec 14, 2023
16 of 17 checks passed
@kitten kitten deleted the @kitten/metro/fix-sourcemap-transform-tests branch December 14, 2023 09:55
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Dec 15, 2023
onizam95 pushed a commit to onizam95/expo-av-drm that referenced this pull request Jan 15, 2024
…-worker with mappings tests (expo#25872)

# Why

- Closes ENG-10873
- This reverts the lock on `@babel/generator@~7.20.5` to
`@babel/generator@^7.20.5` to allow upgrades past `7.21.0`
- This was previously put in place in expo#25833 and this is a follow-up PR
- `@babel/generator@7.21.0` shipped a fix that added names of “friendly
call frames” to its source maps
(babel/babel#15022). This caused the source maps
to change to include internal names for in-scope identifiers such as
`globalThis` and `_interopRequireDefault`
- This was a completely backwards-compatible change for us, but this was
obscured by source map output being a little hard to read

When applied, this PR replaces our source map snapshots with tests that
check exact identifiers in the input and output against their mappings
and names.
This should make it easier to identify changes to the source maps and
allow us to debug them more easily in the future.

A note has been added to any assertions in the tests that break when
downgrading below `7.21.0`, but for now, I haven't added any additional
tests that account for this, since I'm not expecting us to downgrade to
the lower version again, and to instead keep all `@babel/*` packages in
`@expo/metro-config` upgraded in lockstep.

# How

- All source-map snapshots for `metro-transformer-worker` have been
deleted
- `@jridgewell/trace-mapping` has been added to parse the worker’s
source maps (with a small converter function)
- This is an alternative to the ubiquitous `source-map` package, and
it’s also used in Babel. It avoids parsing source maps in wasm.
- When adding `source-map` instead, we’d have to work around our virtual
FS mock
- All source-map snapshots have been replaced with checks for both input
and output identifiers against the actual source-maps
- Additional tests have been added to check for internal globals, which
previously caused the source maps to break

# Test Plan

**Note:** When testing this, placing the pinned version back to
`@babel/generator@~7.20.5` causes some `name` properties to be missing
in the source-map output for injected globals, as expected.

# Checklist

- [ ] 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: fingerprint changed bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants