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-dev): fix linked sourcemap to enabled breakpoints for debugging #2065

Merged
merged 6 commits into from
Mar 31, 2022

Conversation

kiliman
Copy link
Collaborator

@kiliman kiliman commented Feb 21, 2022

There are 2 issues with linked sourcemaps for server build:

  1. sourceMappingURL uses absolute path /build
  2. map file uses route: prefix in route modules so breakpoints are not found

This PR fixes both issues so sourcemap debugging works properly.

Closes #367

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Feb 21, 2022

Hi @kiliman,

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Feb 21, 2022

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

@jacob-ebey
Copy link
Member

Do the inline once with the inclusion of "source-map-support" in @remix-run/node not work for you?

@kiliman
Copy link
Collaborator Author

kiliman commented Feb 22, 2022

It works fine for getting the correct stacktrace. But the issue is that your synthetic "route" module is adding the route: prefix. So when you try to set a breakpoint on the loader for example, VS Code isn't finding the correct file. By stripping the route: prefix from the sources list, everything works fine.

I switched back to "linked" sourcemaps vs "inline" because it was simpler. I didn't have to decode the base64 and back again. Also, I think having external sourcemaps will reduce the server bundle, especially when Cloudflare Workers is already complaining about hitting some limit.

@chaance chaance changed the title Fix linked sourcemap to enabled breakpoints for debugging fix: Fix linked sourcemap to enabled breakpoints for debugging Feb 23, 2022
@chris-kruining
Copy link

chris-kruining commented Mar 1, 2022

should/could the client bundle sourcemaps get a similar treatment?

my error page is working fine with the server bundle now, but is confused about: http://localhost:3000/build/routes/__index/cms-N234CZLH.js:6090

It isn't superhard to reverse engineer the original callsite from this with a sourcemap lib and some clever regex. but if the client side errors could give me a stacktrace that is pointing to tsx files just like with the server bundle would be awesome and save me a bunch of work :D

to illustrate:

server error

Error: Failed to load files
    at loader4 (E:\App\Site\build\route:E:\App\Site\app\routes\__index\cms.tsx:74:15)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at Object.callRouteLoader (E:\App\Site\node_modules\@remix-run\server-runtime\data.js:73:14)

client error

TypeError: Cannot destructure property 'files' of 'useLoaderData(...)' as it is undefined.
    at Cms (http://localhost:3000/build/routes/__index/cms-N234CZLH.js:6090:11)
    at renderWithHooks (http://localhost:3000/build/entry.client-HEZCHDZC.js:11070:26)
    at mountIndeterminateComponent (http://localhost:3000/build/entry.client-HEZCHDZC.js:13190:21)
    at beginWork (http://localhost:3000/build/entry.client-HEZCHDZC.js:13977:22)
    at HTMLUnknownElement.callCallback2 (http://localhost:3000/build/entry.client-HEZCHDZC.js:3680:22)
    at Object.invokeGuardedCallbackDev (http://localhost:3000/build/entry.client-HEZCHDZC.js:3705:24)
    at invokeGuardedCallback (http://localhost:3000/build/entry.client-HEZCHDZC.js:3739:39)
    at beginWork$1 (http://localhost:3000/build/entry.client-HEZCHDZC.js:17086:15)
    at performUnitOfWork (http://localhost:3000/build/entry.client-HEZCHDZC.js:16314:20)
    at workLoopSync (http://localhost:3000/build/entry.client-HEZCHDZC.js:16268:13)

(this is in 1.2.2 btw, just saw that 1.2.3 is released, gonna update soon)
(just updated, same behavior)

@kiliman
Copy link
Collaborator Author

kiliman commented Mar 3, 2022

@jacob-ebey Any update if this PR will be merged?

@kiliman
Copy link
Collaborator Author

kiliman commented Mar 18, 2022

I've rebased my branch to latest dev. Any chance this will be merged soon? I'm currently using patch-package in all my projects to enable this.

@machour machour added the feat:dx Issues related to the developer experience label Mar 20, 2022
@machour
Copy link
Collaborator

machour commented Mar 20, 2022

@chris-kruining please open a new issue for what you're reporting 🙏🏼

kiliman added 2 commits March 21, 2022 11:09
There are 2 issues with linked sourcemaps for server build:

1) sourceMappingURL uses absolute path /build
2) map file uses route: prefix in route modules so breakpoints are not found

This PR fixes both issues so sourcemap debugging works properly.
@kiliman kiliman force-pushed the features/sourcemap branch from b14c885 to 748d23b Compare March 21, 2022 15:11
@kiliman
Copy link
Collaborator Author

kiliman commented Mar 21, 2022

BTW: I think updating the contributors.yml file should be done automatically after the PR is merged. It's very hard to keep the PR updated as the contributors file is constantly being modified.

@MichaelDeBoey MichaelDeBoey changed the title fix: Fix linked sourcemap to enabled breakpoints for debugging fix(remix-dev): fix linked sourcemap to enabled breakpoints for debugging Mar 23, 2022
@mcansh
Copy link
Collaborator

mcansh commented Mar 23, 2022

BTW: I think updating the contributors.yml file should be done automatically after the PR is merged. It's very hard to keep the PR updated as the contributors file is constantly being modified.

we thought about a few different ways to do this instead (form on the website for example), but the problem with doing it after the PR is if you don't end up signing and we merged your work already, and we can't just assume you signed if you got your PR merged

Copy link
Collaborator

@mcansh mcansh left a comment

Choose a reason for hiding this comment

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

thanks! I started to tinker with this a bit ago, but never got around to opening a PR for it, so thanks again 🙏

can you take a look as well @jacob-ebey

@jacob-ebey jacob-ebey merged commit c7d2310 into remix-run:dev Mar 31, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

🤖 Hello there,

We just published version v1.3.5-pre.1 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!

@hi-ogawa hi-ogawa mentioned this pull request Apr 24, 2022
5 tasks
aaronpowell pushed a commit to aaronpowell/remix that referenced this pull request May 3, 2022
…ging (remix-run#2065)

Co-authored-by: Jacob Ebey <jacob.ebey@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed feat:dx Issues related to the developer experience package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect sourceMappingURL added to generated server bundle prevents debugging
6 participants