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 remix-routes not transforming source maps #8970

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

IgnusG
Copy link
Contributor

@IgnusG IgnusG commented Mar 4, 2024

Closes #8453

  • Tests - adjusted tests for vite remix-routes

Testing Strategy:

I tested the previously missing source map functionality locally on my project. Unfortunately I'm not quite sure how to add actual tests into the integrations. I tried a few attempts but I could use some guidance on this 🙏

Here's some more context on why I believe this plugin requires source map support while it transforms code (as pointed out by @pcattori in his comment)

image

In the above is a preview of the vite-inspect-tool showing changes the plugin made to the code. As you can see some lines are removed and shifted around making its source maps point to the wrong line after mapping. This small change adds the parameters needed for generating the source map to the removeExports function (based on some other plugins inside the plugin file) and passes them back to rollup along with the code.

Copy link

changeset-bot bot commented Mar 4, 2024

🦋 Changeset detected

Latest commit: 75f28ce

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing 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 Mar 4, 2024

Hi @IgnusG,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@IgnusG IgnusG changed the base branch from main to dev March 4, 2024 21:01
@IgnusG IgnusG force-pushed the fix/remix-routes-source-maps-missing branch from 7aa3c4c to de4fd54 Compare March 4, 2024 21:02
@IgnusG IgnusG force-pushed the fix/remix-routes-source-maps-missing branch from de4fd54 to 475e835 Compare March 4, 2024 21:03
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 4, 2024

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

@pcattori
Copy link
Contributor

pcattori commented Mar 26, 2024

Verified the fix on the default template's app/routes/_index.tsx route using Source Map Visualization. Before this change, I also confirmed with Source Map Visualization that the sourcemaps were incorrect.

Screenshot 2024-03-26 at 2 06 21 PM

@pcattori pcattori force-pushed the fix/remix-routes-source-maps-missing branch from 2f99114 to 75f28ce Compare March 26, 2024 18:15
@pcattori pcattori merged commit 3976575 into remix-run:dev Mar 26, 2024
9 checks passed
@pcattori
Copy link
Contributor

Thanks for the fix @IgnusG ! 🙏

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