Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

refi93
Copy link

@refi93 refi93 commented Mar 14, 2025

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

Copy link

changeset-bot bot commented Mar 14, 2025

🦋 Changeset detected

Latest commit: 24f99d5

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

This PR includes changesets to release 1 package
Name Type
@crxjs/vite-plugin 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

Copy link

vercel bot commented Mar 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
vite-plugin-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 8, 2025 8:42am

Copy link
Contributor

⚠️ Important Notice: CRXJS is seeking new maintainers.

  • New issues and PRs may not receive immediate attention
  • A new maintenance team must establish itself by March 31, 2025 or this repository will be archived on June 1, 2025
  • Learn more about the transition

This is an automated message. Please do not reply to this comment.

@refi93 refi93 changed the title fix: do not use IIFE for contents scripts without imports fix: do not use IIFE for contents scripts that have no imports Mar 14, 2025
@refi93 refi93 changed the title fix: do not use IIFE for contents scripts that have no imports fix: do not use loader for contents scripts that have no imports Mar 15, 2025
@refi93 refi93 force-pushed the fix-content-script-without-imports branch from 5ee6754 to 2ae977a Compare March 19, 2025 12:04
@refi93 refi93 force-pushed the fix-content-script-without-imports branch from 2ae977a to 6148d15 Compare March 19, 2025 12:19
@refi93 refi93 force-pushed the fix-content-script-without-imports branch from 6148d15 to 82f9c85 Compare April 1, 2025 09:42
Copy link
Collaborator

@Toumash Toumash left a 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

@refi93
Copy link
Author

refi93 commented Apr 2, 2025

No changeset file is present, so the e2e tests didnt run here yet

fixed 👍 77abfa8

Copy link
Collaborator

@Toumash Toumash left a 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

@Toumash
Copy link
Collaborator

Toumash commented Apr 7, 2025

ok, tests were fixed in #1001.
lets update this branch with that change

refi93 added 2 commits April 8, 2025 10:41
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
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.

Skip content script loader during build if it has no imports
3 participants