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

[DevTools] Only call SourceMapConsumer.originalPositionFor once #22181

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

jstejada
Copy link
Contributor

Summary

This commit was extracted from my draft profiling commit (#22167). As part of profiling I decided to make a refactor to only call originalPositionFor once. I don't think this actually affects performance much, since only the first call is the really expensive one, but I figured it might be worth still to consolidate extracting all the information we need from the source map into one call.

Test Plan

  • yarn flow
  • yarn test
  • yarn test-build-devtools
  • manual test of browser extension

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This change seems good 👍🏼

@bvaughn
Copy link
Contributor

bvaughn commented Aug 27, 2021

I just realized that I'm refactoring this code (#22198) because of the network cache issue, and we're going to clash. I made sure to rebase before starting, but I hadn't realized that this change never got merged. Now I'm pretty far into the refactor. Can we hold off on landing this for now?

@jstejada jstejada force-pushed the original-position-for branch 2 times, most recently from a8c773e to 8da394a Compare August 31, 2021 19:30
@jstejada
Copy link
Contributor Author

jstejada commented Sep 1, 2021

@bvaughn rebased on top of your changes, will land if the build passes unless you have any additional comments

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Seems good 👍🏼

Does it impact perf any? (Presumably not but just curious)

@jstejada
Copy link
Contributor Author

jstejada commented Sep 7, 2021

nope, doesn't seem to

@jstejada jstejada merged commit abbc79d into facebook:main Sep 7, 2021
@jstejada jstejada deleted the original-position-for branch September 7, 2021 15:37
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants