-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: expose paths to service worker #8472
Conversation
🦋 Changeset detectedLatest commit: 860ddee The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Thank you! I added the needed types. @Rich-Harris correct me if I'm wrong but I think this also fixes #2571 ? It seems things have changed quite a bit since then and the service worker stuff is since generated through Vite. |
@sellenth can you run |
@dummdidumm ah that type must have been hiding in plain sight, thanks for that and the other tweaks you did. @benmccann got it. |
It does not — service workers !== web workers |
Thank you. There is one wrinkle here that I think is worth discussing though — not sure how big a problem it is. When you set a value for With this change, the local preview and the service worker will have different ideas about |
Revisiting this PR: my previous objection is rescinded because it makes no sense. You can't build a service worker when Given that, it doesn't really make sense to expose I implemented all that in #9250, so I'll close this PR. Thanks! |
closes #4714
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.For issue #4714. I couldn't find a clear example of service worker functionality being tested in the existing tests and didn't know if Playwright browser context was the right way to go about things. I did make an example project to verify this functionality is working and it can be found here. Also my IDE warned me that service workers doesn't have an exported member 'paths'. I assumed this was something that would need to be changed in the LSP project.
Lots of unknowns for me. Just starting contributing to open source...