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 (templates): Add source map retrieval fix to Express template #8176

Closed
wants to merge 1 commit into from

Conversation

kiliman
Copy link
Collaborator

@kiliman kiliman commented Nov 29, 2023

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.

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.
Copy link

changeset-bot bot commented Nov 29, 2023

⚠️ No Changeset found

Latest commit: 9a4596a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@brophdawg11
Copy link
Contributor

What's the best way to recreate the original issue just so I can give this a quick test locally? I assume this impacts server-side sourcemaps? I tried putting a debugger in a route component and loader and using the VSCode debugger but it took me to the correct source file before and after an HMR update.

@kiliman
Copy link
Collaborator Author

kiliman commented Nov 30, 2023

Yeah, I wasn't sure how to do an automated test for this.

This is how I'm testing now:

// routes/_index.tsx
import { Form } from "@remix-run/react";

export async function action() {
  throw new Error("Oops!");
}

export default function Index() {
  return (
    <div>
      <h1>Welcome to Remix</h1>
      <Form method="post">
        <button>Submit</button>
      </Form>
    </div>
  );
}

Click on the submit button. It should throw a server-side exception.

Check the stack trace to make sure it shows the actual source file and not the compiled version.

The issue shows up when you make a change that causes Remix to rebuild (imports index.js?t=timestamp).

Make a change then press the submit button again.

It should still show the correct stack trace. Without the fix, it will show the compiled version since it didn't know how to find the map file for the file with the timestamp suffix.

Also, this doesn't affect debugging. The issue is with the stack trace when logging the error via source-map-support.

NOTE: There's PR #8174 that fixes this for remix-serve. And there's a discussion that talks about the problem and the solution #8168.

@brophdawg11
Copy link
Contributor

Ah ok thanks @kiliman. Let's just use the other PR since it updates both spots together 👍

@brophdawg11 brophdawg11 closed this Dec 1, 2023
@kiliman
Copy link
Collaborator Author

kiliman commented Dec 1, 2023

Yeah, sorry I messed up the PRs. It's been awhile.

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.

2 participants