-
Notifications
You must be signed in to change notification settings - Fork 213
fix: do not use loader for contents scripts that have no imports #997
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
base: main
Are you sure you want to change the base?
fix: do not use loader for contents scripts that have no imports #997
Conversation
🦋 Changeset detectedLatest commit: 24f99d5 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This is an automated message. Please do not reply to this comment. |
5ee6754
to
2ae977a
Compare
2ae977a
to
6148d15
Compare
6148d15
to
82f9c85
Compare
e8700e9
to
6629d0e
Compare
6629d0e
to
69b30f4
Compare
69b30f4
to
bc06235
Compare
bc06235
to
69209bb
Compare
69209bb
to
0921ff9
Compare
0921ff9
to
b595737
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, still needs to be run locally to test.
❌No changeset file is present, so the e2e tests didnt run here yet
4611cfd
to
794b97d
Compare
fixed 👍 77abfa8 |
794b97d
to
77abfa8
Compare
21cabd3
to
c1db3a3
Compare
...es/vite-plugin/tests/e2e/mv3-vite-svelte-content-script/__image_snapshots__/vite-build-0.png
Outdated
Show resolved
Hide resolved
packages/vite-plugin/tests/out/vite-svelte/__snapshots__/serve.test.ts.snap
Outdated
Show resolved
Hide resolved
packages/vite-plugin/tests/out/with-sourcemaps/__snapshots__/build.test.ts.snap
Show resolved
Hide resolved
packages/vite-plugin/tests/out/vite-react-fast-refresh/__snapshots__/serve.test.ts.snap
Outdated
Show resolved
Hide resolved
...ges/vite-plugin/tests/out/vite-content-script-css-imports-2/__snapshots__/build.test.ts.snap
Show resolved
Hide resolved
f6755cc
to
b0a38d9
Compare
...es/vite-plugin/tests/e2e/mv3-vite-svelte-content-script/__image_snapshots__/vite-build-0.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 IMO ok
but not ready to be merged yet tho
we need to either:
- wait for the fix: make HMR working again with latest vite version #999 to merge to have the image snapshot tests resolved
- apply the same fixes in this PR
- ➡️create a separate PR that resolves the image snapshots first (so we'll not block any other PRs and will be super easy to merge)
Lets create a separate PR #1001 and wait for it to merge
ok, tests were fixed in #1001. |
b0a38d9
to
0092f61
Compare
Motivation: wrapping in IIFE may delay script injection, which is a needless tradeoff if the script has no (ES module) imports inspired by https://github.com/samrum/vite-plugin-web-extension/blob/main/src/utils/loader.ts#L57-L66
0092f61
to
24f99d5
Compare
Motivation: content script loader may delay script injection, which is a needless tradeoff if the script has no (ES module) imports
inspired by https://github.com/samrum/vite-plugin-web-extension/blob/main/src/utils/loader.ts#L57-L66
closes #996