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

feat: adapter-vercel should generate sourcemaps #5197

Merged
merged 4 commits into from
Jun 24, 2022

Conversation

ACHP
Copy link
Contributor

@ACHP ACHP commented Jun 11, 2022

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it. (adapter-vercel package does not have tests)

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

Why

This pull-request adds a new option to enable sourcemap for the generated build. This is really important for error monitoring integration. (eg; Sentry in my case).
For now there is no easy way to debug a production app deployed with sveltkit on vercel.

@changeset-bot
Copy link

changeset-bot bot commented Jun 11, 2022

🦋 Changeset detected

Latest commit: 9cd0a34

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-vercel 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

@ACHP ACHP changed the title feat: adapter-vercel should be able to generate sourcemap patch: adapter-vercel should be able to generate sourcemap Jun 11, 2022
@ACHP ACHP changed the title patch: adapter-vercel should be able to generate sourcemap feat: adapter-vercel should be able to generate sourcemap Jun 11, 2022
@Rich-Harris
Copy link
Member

Thank you! Is there any reason to make this optional? Why wouldn't we always generate it?

@benmccann
Copy link
Member

We probably should always generate sourcemaps, but I wonder what the default should be. It's not a simple boolean but an enum. To generate them as separate files probably is a good default, but I could see people wanting something different

@Rich-Harris
Copy link
Member

I could see people wanting something different

I can't. I think we should just always do linked sourcemaps (i.e. true) — if someone comes up with a compelling rationale for exposing an option, we could easily add it in a non-breaking way in future, but until then I think we should abide by the principle that configuration is a last resort

@benmccann
Copy link
Member

Making it linked until someone needs something else sounds like a fine solution to me

ACHP added 4 commits June 22, 2022 20:14
The adapter-vercel package were not generating sourcemaps. Therefore, it was impossible to correctly use any bug tracking tool.
@ACHP
Copy link
Contributor Author

ACHP commented Jun 22, 2022

Thank you for your reviews, I updated this PR according to your comments. However the CI seems broken on Windows but I don't think it's related to this pull-request

@ACHP ACHP changed the title feat: adapter-vercel should be able to generate sourcemap feat: adapter-vercel should be generate sourcemaps Jun 22, 2022
@ACHP ACHP changed the title feat: adapter-vercel should be generate sourcemaps feat: adapter-vercel should generate sourcemaps Jun 22, 2022
@benmccann
Copy link
Member

I reran the Windows CI and it passed. I wouldn't worry if it's only one test job that fails. We have some flaky tests

@benmccann
Copy link
Member

My one other question is whether we also need to make the same change to the other adapters that use esbuild. I'm guessing we probably do, but haven't checked

@ACHP
Copy link
Contributor Author

ACHP commented Jun 23, 2022

My one other question is whether we also need to make the same change to the other adapters that use esbuild. I'm guessing we probably do, but haven't checked

Yes, there is no reason not to do it on other adapters I guess. I did it on the vercel adapter because this is what I'm working on at the moment and it's easy for me to test it, so I'm was confident enought to do it. Do you think other adapters should be updated in this PR as well or should we merge it as is and create other pull-request for different adapters ?

@benmccann
Copy link
Member

I'd rather we just do them all in this PR if you don't mind updating it. Only a few of the adapters use esbuild, so hopefully there won't be too much to update.

@8462852
Copy link

8462852 commented Jun 23, 2022

I think this should be configurable (on by default). Reason: Source code protection. I know a large company that uses kit in production that needs this option.

@benmccann
Copy link
Member

You don't have to provide the source maps to anyone outside your company, so I don't see that as a reason to make it configurable

@8462852
Copy link

8462852 commented Jun 23, 2022

You don't have to provide the source maps to anyone outside your company, so I don't see that as a reason to make it configurable

Ok thanks. I just don't understand how I could prevent to ship source maps in the current proposal. Would I need to fork the Vercel adapter?

@Rich-Harris
Copy link
Member

@8462852 to be clear, we're only talking about sourcemaps for your serverless/edge functions. The sourcemapped stack traces will not be visible to anyone who doesn't have access to https://vercel.com/YOUR_ORG/YOUR_PROJECT/YOUR_DEPLOYMENT_ID/functions?name=YOUR_FUNCTION

@Rich-Harris
Copy link
Member

thank you!

@benmccann benmccann mentioned this pull request Jun 24, 2022
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