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

download correct sourceMap for dev/prod profiles, add warning for Expo Dev menu profiles #1833

Merged

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Feb 16, 2023

This fixes #1831

Summary:

As described over in #1831, until now the profile-hermes command would always load the dev mode soureMaps, even if a production build was being profiled - this lead to wrong line numbers (at best).

This PR detects if the profile has been recorded with a dev or prod build, which platform was used and if minify was enabled.
Unfortunately, at this point, something further down the line seems to fail with a profile that was produced for a minifed build, so I could not really test that - that seems to be some problem with the general process and not with my changes, though.

With minify: false, I verified this to work correctly for both dev=true and dev=false.

If no URL information is present in the profiler, the PR just defaults to the values that were in the code before this change.

This PR also adds a warning if no URL information is present, but the profile contains calls to EXDevMenuApp. That is usually the case when the profiler does not record the user application, but the Expo dev menu. In that case, the user has to reload their app a few times before recording another profile - Hermes always profiles the JS bundle that was loaded last. I'm adding this warning because this is documented nowhere else, and it cost me a day until I found this out for myself.

Test Plan:

Record Profiles with different combinations of "DEV" and "minify" selected in the settings reachable from the React Native Dev menu, call npx react-native profile-hermes --verbose after each recording and compare the resulting profile line numbers with the actual source code.

add warning for Expo Dev menu profiles
@thymikee
Copy link
Member

@huntie would you mind taking a look at this?

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.

@github-actions github-actions bot added the stale label Jun 20, 2023
@phryneas
Copy link
Contributor Author

This is still absolutely relevant, but the bot is right, it's been 3 months - could someone maybe take a look at this?

@thymikee thymikee requested a review from huntie June 20, 2023 07:50
Copy link
Collaborator

@huntie huntie left a comment

Choose a reason for hiding this comment

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

@phryneas Looks good to me ✅ Sorry about the delay on this review! 🙏🏻

@thymikee thymikee merged commit a1cc798 into react-native-community:main Jun 20, 2023
szymonrybczak pushed a commit that referenced this pull request Feb 19, 2024
add warning for Expo Dev menu profiles

Co-authored-by: Lenz Weber-Tronic <lenz@apollographql.com>
thymikee pushed a commit that referenced this pull request Feb 19, 2024
add warning for Expo Dev menu profiles

Co-authored-by: Lenz Weber-Tronic <lenz@apollographql.com>
szymonrybczak pushed a commit that referenced this pull request Feb 19, 2024
add warning for Expo Dev menu profiles

Co-authored-by: Lenz Weber-Tronic <lenz@apollographql.com>
thymikee pushed a commit that referenced this pull request Feb 19, 2024
add warning for Expo Dev menu profiles

Co-authored-by: Lenz Weber-Tronic <lenz@apollographql.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

profile-hermes command always downloads dev sourceMaps
3 participants