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: apply custom serializer to source map endpoint in dev server #1284

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EvanBacon
Copy link
Contributor

@EvanBacon EvanBacon commented Jun 5, 2024

Summary

Changelog: [Fix] Apply custom serializer to source map graph in .map endpoint.

@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 Jun 5, 2024
@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 Jun 5, 2024
Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

This doesn't address the same issue occurring in error symbolication

Yeah we should definitely make sure those are all consistent.

Comment on lines +1156 to +1157
// If the project defines a custom serializer, then we should run it to ensure any
// mutations to the graph are applied in the sourcemap.
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to me to run the custom serializer consistently (and I vaguely remember seeing cases where this was broken), but what do you mean by "mutations to the graph"? When during bundling do those occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone were to preModules.splice(0,0, { ... }) to add a new module inside of a custom serializer, then it would technically mutate the arguments used to generate the bundle. I can think of two cases like this:

  • sentry (not exactly, but close)
  • One I just wrote for Expo CLI as a fallback to returning the map: here

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. The serializer API wasn't designed with side-effectful serializers in mind, and the preModules parameter specifically is typed as being read-only. Is there a way to achieve what you're trying to do without mutating read-only arguments?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but it's a little unstable. I'm sniffing if the sourceUrl has a .map pathname to determine if the serializer should return source maps. This works for the .map endpoint, but it doesn't account for /symbolicate where the result is getExplodedSourceMap and not a standard source map string.

@hezi hezi requested a review from robhogan June 5, 2024 16:23
EvanBacon added a commit to expo/expo that referenced this pull request Jun 6, 2024
…9463)

# Why

- The source maps are off-by-1 in development and this causes debugging
and errors to be janky.
- The cause is related to Metro serializing the graph differently in
`.map` requests than it does in `.bundle` requests.
- Required change in Metro facebook/metro#1284

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Mutate the premodules to try and mitigate issues like this from
happening in the future.
- Detect when a serialization request is meant for source maps in
development, and return the maps from the serializer. This depends on an
upstream fix in Metro (if it doesn't land, we can try something else).
- [Update]: I've also added a new system (consider existing
implementation legacy) which injects a polyfill that is one line long.
We then update this polyfill in the serializer accordingly to ensure
source maps stay correct, even in cases such as error handling where
Metro serializes them a third way.


<!--
How did you build this feature or fix this bug and why?
-->

# Test Plan

- Added an E2E dev server test which pings the source maps and detects
if the `__env__` module is included.
- Physically attached a debugger and ensured that the sources were
aligned in development.

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] 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).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
EvanBacon added a commit to expo/expo that referenced this pull request Jun 10, 2024
…9463)

# Why

- The source maps are off-by-1 in development and this causes debugging
and errors to be janky.
- The cause is related to Metro serializing the graph differently in
`.map` requests than it does in `.bundle` requests.
- Required change in Metro facebook/metro#1284

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Mutate the premodules to try and mitigate issues like this from
happening in the future.
- Detect when a serialization request is meant for source maps in
development, and return the maps from the serializer. This depends on an
upstream fix in Metro (if it doesn't land, we can try something else).
- [Update]: I've also added a new system (consider existing
implementation legacy) which injects a polyfill that is one line long.
We then update this polyfill in the serializer accordingly to ensure
source maps stay correct, even in cases such as error handling where
Metro serializes them a third way.


<!--
How did you build this feature or fix this bug and why?
-->

# Test Plan

- Added an E2E dev server test which pings the source maps and detects
if the `__env__` module is included.
- Physically attached a debugger and ensured that the sources were
aligned in development.

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] 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).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.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. 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.

3 participants