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

Use service name as service worker script name #190

Merged
merged 1 commit into from
Jan 16, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Nov 24, 2022

Hey! 👋 Currently, when multiple nanoservices are configured with service worker scripts, they'll all use worker.js as their script origin. This means if an error is thrown, it's difficult to identify which script threw from inspecting the stack trace. We hit this issue in Miniflare, when attempting to locate source maps for service workers: https://github.com/cloudflare/miniflare/blob/5ecaae2b482feb583d8eeca2310785ea4d68165c/packages/tre/src/plugins/core/prettyerror.ts#L37-L41.

This PR adds a new name option for configuring the script origin of service worker scripts. Rather than making serviceWorkerScript a :group with name, I've added a new serviceWorker union member to maintain text-format compatibility.

Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

If providing a filename is really the right approach, this seems like the best way to do it.

But I have to wonder about this:

This means if an error is thrown, it's difficult to identify which script threw from inspecting the stack trace.

If there's only one script, why is it difficult to identify?

Is the problem really that separate services are not distinguished from each other? If so, would an alternative option be to bake the service name into the script's display name? That could happen automatically and would benefit everyone.

If that solves the problem for miniflare, and miniflare is then able to apply source mapping to map locations back to their original input source code, then do we still need to add this filename option? I just feel like no one would ever bother specifying it when writing a worker by hand, and tool-generated configs don't really need it if they are applying source mapping on top anyway.

src/workerd/server/workerd.capnp Outdated Show resolved Hide resolved
src/workerd/server/workerd.capnp Outdated Show resolved Hide resolved
@mrbbot
Copy link
Contributor Author

mrbbot commented Nov 24, 2022

That's a good point.

Is the problem really that separate services are not distinguished from each other?

Yep, this is the problem. Using <worker_name>(.js) as the origin would be enough for us to distinguish services.

However, for users that are just using workerd directly, having the ability to specify the file name might be nice, as clicking on stack trace call sites in IDEs often takes you directly to the code, assuming the names match. As you say though, it doesn't seem likely people would bother specifying this in embed and then again as (file)name. Would definitely prefer if we didn't need two ways of defining service worker scripts too.

@mrbbot mrbbot force-pushed the bcoll/service-worker-name branch from 3386aa7 to f455aba Compare November 25, 2022 11:37
@mrbbot
Copy link
Contributor Author

mrbbot commented Nov 25, 2022

Ok, I've switched to using the service name as the script name. This is sufficient for Miniflare to distinguish scripts. If users want IDE click-to-location in stack traces, they can set the service name to their filename. 👍 This needs a small internal change too, so I'll make the PR for that now.

@mrbbot mrbbot changed the title Add service worker script name configuration Use service name as service worker script name Nov 25, 2022
@jasnell
Copy link
Member

jasnell commented Nov 29, 2022

LGTM but let's also have @kentonv review.

@mrbbot mrbbot force-pushed the bcoll/service-worker-name branch from f455aba to 23eed29 Compare December 7, 2022 10:56
@mrbbot mrbbot force-pushed the bcoll/service-worker-name branch from 23eed29 to 8d97864 Compare December 14, 2022 18:19
@jasnell
Copy link
Member

jasnell commented Dec 15, 2022

@mrbbot ... I left a comment on the internal repo also but the CI run for this is fairly old. Can I ask you to rebase this and the internal repo so we can get a fresh CI run before we merge this.

@mrbbot
Copy link
Contributor Author

mrbbot commented Dec 16, 2022

Sure! I'm away from my laptop for the next couple days, but will do this when I get back. 👍

@mrbbot mrbbot force-pushed the bcoll/service-worker-name branch from 8d97864 to 3e868d5 Compare December 19, 2022 12:47
@mrbbot mrbbot force-pushed the bcoll/service-worker-name branch from 3e868d5 to c081fdd Compare January 5, 2023 10:35
@mrbbot mrbbot force-pushed the bcoll/service-worker-name branch from c081fdd to 77d8204 Compare January 16, 2023 09:23
@dom96 dom96 merged commit b566e3a into main Jan 16, 2023
@mrbbot mrbbot deleted the bcoll/service-worker-name branch February 21, 2023 11:14
mrbbot added a commit to cloudflare/miniflare that referenced this pull request May 10, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/miniflare that referenced this pull request May 10, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/miniflare that referenced this pull request May 12, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/miniflare that referenced this pull request May 15, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/miniflare that referenced this pull request May 15, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/workers-sdk that referenced this pull request Oct 31, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/workers-sdk that referenced this pull request Nov 1, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/workers-sdk that referenced this pull request Nov 1, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
mrbbot added a commit to cloudflare/workers-sdk that referenced this pull request Nov 1, 2023
cloudflare/workerd#190 changed the script name for service workers to
their service name. This change updates Miniflare's heuristics for
locating source maps to work with this. It also adds some tests to
ensure source mapping is working correctly for the different ways of
specifying a script.
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.

4 participants