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(worker): Add sourcemap support for worker bundles #5417

Merged
merged 13 commits into from Mar 29, 2022

Conversation

KallynGowdy
Copy link
Contributor

Description

Improves the built-in worker plugin to support emitting sourcemaps for generated worker bundles. Closes #5395.

Tests have been added to validate that sourcemaps are generated for inline, non-inline, and shared workers.

Additional context

3 additonal test packages have been added:

  • sourcemap
  • sourcemap-hidden
  • sourcemap-inline

Each of these packages are copied from the worker test package and adds a vite.config.ts to enable sourcemaps for the respective test case (true, hidden, and inline). The actual test code validates that the .js.map files are created and additionally that the source files link to them using a //# sourceMappingURL= comment.

One tricky edge case is the scenario where non-inline sourcemaps are being used with inline workers. In this case, the embedded comment has to use the full path (basepath + fileName) instead of simply using the file name. The tests don't address this case, partly because querying the inlined worker code would be pretty messy and it is something where manually running the playground catches the issue pretty easily.

Other details are mentioned via comments in worker.ts.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

- Supports true, "inline", and "hidden" for config.build.sourcemap options.
- Supports inline and non-inline workers
@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Oct 25, 2021
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@Crisfole
Copy link

Crisfole commented Nov 19, 2021

@Shinigami92 It looks like this is working but might be waiting on mac/windows servers to run? And has been for a really long time. What would this take to get moving forward?

== EDIT ==

To be clear, I mean this as "Is this a CI server resourcing issue? Or just someone needing to click a button?"

Not as "it's your responsibility"

@Shinigami92
Copy link
Member

You need to update the branch with the newest main

@KallynGowdy
Copy link
Contributor Author

@Shinigami92 Should I do that?

@Shinigami92
Copy link
Member

Would be nice :)

@Crisfole
Copy link

@KallynGowdy I just checked: rebase and merge both happen without incident!

@Shinigami92
Copy link
Member

uwu 🙁 seems there is an issue with windows?!

@Shinigami92
Copy link
Member

Mh I will let rerun the tests, could be an issue in CI not your fault 🤔

Shinigami92
Shinigami92 previously approved these changes Nov 19, 2021
@KallynGowdy
Copy link
Contributor Author

@Shinigami92 Is there any update on this?

@poyoho
Copy link
Member

poyoho commented Mar 26, 2022

hi @KallynGowdy. Are you interested in continuing working on it? if not I can fork the PR and let it forward. ❤️

@KallynGowdy
Copy link
Contributor Author

@poyoho Yes, I can update it. Thanks for the offer though!

@poyoho
Copy link
Member

poyoho commented Mar 28, 2022

@poyoho Yes, I can update it. Thanks for the offer though!

I think you can try to think about my unit test writing method 😊, And add the nested worker maybe had a micro error.

@patak-dev patak-dev added this to the 3.0 milestone Mar 28, 2022
…cemaps

# Conflicts:
#	packages/vite/src/node/plugins/worker.ts
#	packages/vite/src/node/plugins/workerImportMetaUrl.ts
packages/vite/src/node/plugins/worker.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/worker.ts Outdated Show resolved Hide resolved
poyoho
poyoho previously approved these changes Mar 28, 2022
Copy link
Member

@poyoho poyoho left a comment

Choose a reason for hiding this comment

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

look good to me. great work here!

@KallynGowdy
Copy link
Contributor Author

Looks like another flaky test run...

@poyoho
Copy link
Member

poyoho commented Mar 29, 2022

There should be no problem. I often encounter this situation 🙆‍♂️

…cemaps

# Conflicts:
#	packages/vite/src/node/plugins/worker.ts
#	packages/vite/src/node/plugins/workerImportMetaUrl.ts
@patak-dev
Copy link
Member

Awesome work @KallynGowdy, let's get this one in 2.9!

@patak-dev patak-dev merged commit 465b6b9 into vitejs:main Mar 29, 2022
@patak-dev patak-dev modified the milestones: 3.0, 2.9 Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: web workers p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sourcemaps for Web Worker bundles
6 participants