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-serve): Fix source map loading #8174

Merged
merged 4 commits into from Dec 4, 2023
Merged

Conversation

kiliman
Copy link
Collaborator

@kiliman kiliman commented Nov 29, 2023

This PR fixes the issue where source maps are not getting used. This was traced to how the map file is loaded when HMR reloads the server file with the ?t=timestamp suffix.

This causes the source map to not be found and therefore not used.

Adding the hook for retrieveSourceMap, this strips out the unexpected characters from the filename and properly returns the source map.

Closes: #8168.

This PR fixes the issue where source maps are not getting used. This was traced to how the map file is loaded when HMR reloads the server file with the `?t=timestamp` suffix.

This causes the source map to not be found and therefore not used.

Adding the hook for `retrieveSourceMap`, this strips out the unexpected characters from the filename and properly returns the soure map.
Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: 404de6a

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/serve Patch
@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/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

This PR updates the the Express template and fixes the issue where source maps are not getting used. This was traced to how the map file is loaded when HMR reloads the server file with the `?t=timestamp` suffix.

This causes the source map to not be found and therefore not used.

Adding the hook for `retrieveSourceMap`, this strips out the unexpected characters from the filename and properly returns the source map.
@kiliman
Copy link
Collaborator Author

kiliman commented Nov 29, 2023

Ugh. I created 2 PRs back-to-back, one for Remix App Server and one for the Express template. I forgot to branch off of upstream/dev on the 2nd PR, so that template fix is added to this one.

I created the 2nd PR, which only has the template fix, but if you merge this one, that PR is obviated. Since these two are related, I don't think it would be an issue.

Anyway, let me know how you guys want to handle this. Sorry, it's been a while doing PRs.

@Alex-Levacher
Copy link

@kiliman Thank you so much for the fix 💌

kiliman added a commit to kiliman/epic-stack that referenced this pull request Nov 30, 2023
This PR adds the fix submitted in Remix PR #8174. The issue is caused when the build server imports an updated server build with a ?t=timestamp suffix. This causes the source map support library to fail to load the source map.

remix-run/remix#8174
kentcdodds added a commit to epicweb-dev/epic-stack that referenced this pull request Nov 30, 2023
* fix (server): Fix source map retrieval for updated builds

This PR adds the fix submitted in Remix PR #8174. The issue is caused when the build server imports an updated server build with a ?t=timestamp suffix. This causes the source map support library to fail to load the source map.

remix-run/remix#8174

* Update index.js

* fix lint/build issues

---------

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
@brophdawg11
Copy link
Contributor

This looks good to me on both express and remix-serve. Here's a screenshot of the before/after (I applied these changes to remix-serve directly in node_modules and restarted:

Screenshot 2023-12-01 at 9 11 25 AM

I'm going to kick this over to @pcattori and @jacob-ebey for a confirmation though since they're more in tune with the HMR + source maps flows.

@pcattori pcattori merged commit 999b565 into remix-run:dev Dec 4, 2023
5 checks passed
Copy link
Contributor

github-actions bot commented Dec 5, 2023

🤖 Hello there,

We just published version 2.4.0-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!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.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!

@IgnisDa
Copy link

IgnisDa commented Dec 17, 2023

Hi @kiliman! This PR seems to have introduced a new bug, as mentioned in the above issue. I would be really grateful if you could take a look.

@kiliman
Copy link
Collaborator Author

kiliman commented Dec 18, 2023

Thanks. I'll take a look in the morning.

@kiliman
Copy link
Collaborator Author

kiliman commented Dec 18, 2023

@IgnisDa this is fixed in PR #8321

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

6 participants