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

Improve source map support #3739

Closed

Conversation

ssttevee
Copy link
Contributor

What this PR solves / how to test:
This PR improves on #3140.

There are a few functional changes:

  • Running sample scripts without wrangler.toml now properly translates traces
  • Stack traces are translated before printed to cli output and dev tools console
  • File references in cli output is relative to process.cwd()
  • File references in stack traces in dev tools console is now clickable and reveals the relevant source code
  • Error.stack strings are also parsed and translated
  • SourceMapConsumer is managed in a useEffect and recreated whenever the script is reloaded
  • Network.loadNetworkResource CDP commands can now load source files referenced in translated traces

Dev Tools Before:
image

Dev Tools After:
image

Terminal Before:
image

Terminal After:
image

To test:

  1. Run npm start -w wrangler dev ../../fixtures/worker-ts/src/index.ts
  2. Press 'd' to open dev tools
  3. Run curl localhost:8787/error
  4. See translated stack traces

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

@ssttevee ssttevee requested review from a team as code owners August 11, 2023 16:39
@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2023

🦋 Changeset detected

Latest commit: 513ab63

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

This PR includes changesets to release 1 package
Name Type
wrangler 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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5860269554/npm-package-wrangler-3739

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5860269554/npm-package-wrangler-3739

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5860269554/npm-package-wrangler-3739 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5860269554/npm-package-cloudflare-pages-shared-3739

Note that these links will no longer work once the GitHub Actions artifact expires.

@mrbbot
Copy link
Contributor

mrbbot commented Aug 14, 2023

Hey! 👋 Thank you opening this PR! This looks like a big improvement in developer experience. 😃 Would you be able to take a look at the failing checks?

@ssttevee ssttevee force-pushed the more-source-map-improvements branch from f4c7334 to f97228c Compare August 14, 2023 16:32
@ssttevee
Copy link
Contributor Author

I fixed the lint issues, but the only failed test that I see is a timeout issue.

@Skye-31
Copy link
Contributor

Skye-31 commented Aug 14, 2023

We probably don't want the changeset for worker-ts here - the change is for wrangler

@ssttevee ssttevee force-pushed the more-source-map-improvements branch from f97228c to af17af7 Compare August 14, 2023 20:17
@ssttevee ssttevee force-pushed the more-source-map-improvements branch from af17af7 to 513ab63 Compare August 14, 2023 20:19
@ssttevee
Copy link
Contributor Author

That's a good point, I removed it.

@Skye-31
Copy link
Contributor

Skye-31 commented Aug 15, 2023

Looks like the pages-workerjs-app test:ci script is failing for some reason

@mrbbot
Copy link
Contributor

mrbbot commented Aug 15, 2023

Hey again! 👋 workerd recently added support for breakpoint debugging. As part of this work, source mapping support within the runtime has improved. I'm planning to add support for breakpoint debugging to Wrangler this week, using the runtime's native support for source mapping, which may solve some of these problems. I suspect we'll still need this PR in certain cases, and certainly in remote mode too. 🙂 I'm going to hold off reviewing this until the debugging work is completed, and we know what we need here. Thanks again for the PR! 👍

@ssttevee
Copy link
Contributor Author

The pages-workerjs-app test:ci script seems to be failing inconsistently on main as well

@jbergstroem
Copy link

For me (on MacOS, Wrangler 3.9.0), the stacktraces are from the caches folder. Not super helpful:

[mf:inf] GET / 500 Internal Server Error (22ms)
TypeError: Cannot read properties of undefined (reading 'responseSchema')
    at file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:13862:67
    at Array.map (<anonymous>)
    at getConfig (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:13850:24)
    at Object.fetch (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:14167:28)
    at __facade_modules_fetch__ (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:14291:46)
    at __facade_invokeChain__ (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:5531:10)
    at Object.next (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:5528:14)
    at jsonError (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:14249:32)
    at __facade_invokeChain__ (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:5531:10)
    at __facade_invoke__ (file:///private/var/folders/zz/850k5ry57k7_c82_k26zdpl00000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:5534:10) {
  stack: TypeError: Cannot read properties of undefined (re…0000gn/T/tmp-87753-l9fe9gWNMnJB/index.js:5534:10),
  message: Cannot read properties of undefined (reading 'responseSchema')

@lrapoport-cf lrapoport-cf added the awaiting Cloudflare response Awaiting response from workers-sdk maintainer team label Sep 29, 2023
@mrbbot
Copy link
Contributor

mrbbot commented Sep 29, 2023

Hey @ssttevee! 👋 The breakpoint debugging work is now pretty much complete. As part of this, we simplified and improved lots of our source mapping logic. From your list of functional changes, I think the only things still missing are:

  • Stack traces are translated before printed to cli output and dev tools console
  • File references in cli output is relative to process.cwd()
  • Error.stack strings are also parsed and translated

Given the code this PR touches has changed significantly, we think it would be unfair to ask you to update this PR. As such, we're planning to implement these changes ourselves in the next few working days, using parts of this PR if possible. Would you prefer we made these changes in your fork, or on a new branch? We'll make sure you're a co-author on any commits regardless. 🙂

@mrbbot
Copy link
Contributor

mrbbot commented Sep 29, 2023

@jbergstroem, would you be able to open a new issue for this?

@lrapoport-cf
Copy link
Contributor

hi @ssttevee 👋 just following up on @mrbbot 's comment -- would you prefer we made these changes in your fork, or on a new branch? if we don't hear from you in the next couple of days we'll just go ahead and do the work on a new branch, but let us know if you prefer otherwise! also as @mrbbot noted, we'll make sure you're a co-author on any commits regardless :)

@lrapoport-cf lrapoport-cf added awaiting reporter response Needs clarification or followup from OP and removed awaiting Cloudflare response Awaiting response from workers-sdk maintainer team labels Oct 6, 2023
@ssttevee
Copy link
Contributor Author

Sorry I missed this, but I wouldn't mind either way and thanks for keeping me as a co-author. :)

There was one more thing I wanted to address in this pr that I never got around to, which is debugging support in vscode.

@lrapoport-cf lrapoport-cf added awaiting Cloudflare response Awaiting response from workers-sdk maintainer team and removed awaiting reporter response Needs clarification or followup from OP labels Oct 20, 2023
@mrbbot
Copy link
Contributor

mrbbot commented Nov 16, 2023

Hey! 👋 As an update, we have a draft PR up that should address this remaining requirements (#4423), but it's blocked on #4341 going in first. Once that's landed, we should be able to rebase and merge those fixes. 👍

@1000hz 1000hz removed the awaiting Cloudflare response Awaiting response from workers-sdk maintainer team label Nov 20, 2023
@lrapoport-cf lrapoport-cf removed the blocked Blocked on other work label Nov 27, 2023
@mrbbot
Copy link
Contributor

mrbbot commented Nov 28, 2023

Hey! 👋 #4423 is now ready, and should provide the rest of the functionality added by this PR. Thanks again for submitting this!

@mrbbot mrbbot closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants