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 react-refresh source maps #12686

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

soxtoby
Copy link
Contributor

@soxtoby soxtoby commented Jan 4, 2025

Resolves #12483

The react-router:react-refresh-babel transform was wrapping the code with big chunks of code that completely threw off the source maps. I've changed the wrapping code to use babel to maintain the source maps, and split it off into a new transform step so the source maps from the previous transform flow through.

@remorses
Copy link

remorses commented Jan 4, 2025

I propose another solution: put the react refresh header in a single line, then remove the first new line of the file. This way only the first line sourcemaps will be wrong, which shouldn’t matter anyway because it only contains imports.

this technique is already used by Vite for other transformations, it lets you skip parsing one more time with Babel which is expensive

Copy link

changeset-bot bot commented Jan 4, 2025

🦋 Changeset detected

Latest commit: b35ddb1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@react-router/dev Patch
@react-router/fs-routes Patch
@react-router/remix-routes-option-adapter Patch
create-react-router Patch
react-router Patch
react-router-dom Patch
@react-router/architect Patch
@react-router/cloudflare Patch
@react-router/express Patch
@react-router/node Patch
@react-router/serve Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 4, 2025

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@soxtoby
Copy link
Contributor Author

soxtoby commented Jan 4, 2025

@remorses thanks for suggestion - the output isn't as pretty, but the code is much simpler, and the source maps are even better since the first line of code comes from react-refresh and was already unmapped.

Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

I've run this locally and confirmed it works. Thanks for the PR 🙏

@markdalgleish markdalgleish merged commit 02ade43 into remix-run:dev Jan 7, 2025
1 check passed
Copy link
Contributor

🤖 Hello there,

We just published version 7.1.2-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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.

3 participants