-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
packages/@expo/metro-config/src/transform-worker/__tests__/metro-transform-worker.test.ts
Show resolved
Hide resolved
packages/@expo/metro-config/src/transform-worker/__tests__/metro-transform-worker.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
Be sure to add the changelog entry |
3ef8fdc
to
0690a97
Compare
0690a97
to
2784326
Compare
52a5e3a
to
d358da5
Compare
…-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).
Why
@babel/generator@~7.20.5
to@babel/generator@^7.20.5
to allow upgrades past7.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 asglobalThis
and_interopRequireDefault
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
metro-transformer-worker
have been deleted@jridgewell/trace-mapping
has been added to parse the worker’s source maps (with a small converter function)source-map
package, and it’s also used in Babel. It avoids parsing source maps in wasm.source-map
instead, we’d have to work around our virtual FS mockTest Plan
Note: When testing this, placing the pinned version back to
@babel/generator@~7.20.5
causes somename
properties to be missing in the source-map output for injected globals, as expected.Checklist
npx expo prebuild
& EAS Build (eg: updated a module plugin).