Skip to content

refactor: cleanup esm #1950

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.

Sign up for GitHub

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

Jump to bottom
Open
lishaduck wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor: cleanup esm #1950

lishaduck wants to merge 3 commits into from

Conversation

@lishaduck
Copy link
Contributor

@lishaduck lishaduck commented Oct 24, 2025 •
edited
Loading

Overview

A few tiny cleanups I noticed could be made while perusing around the codebase.

Manual Testing

Bundle, check it still works.

Related Issue

This PR is stacked on #1945.

@netlify
Copy link

netlify bot commented Oct 24, 2025 •
edited
Loading

✅ Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit f85a597
🔍 Latest deploy log https://app.netlify.com/projects/creative-fairy-df92c4/deploys/694897491037da00082cffae
😎 Deploy Preview https://deploy-preview-1950--creative-fairy-df92c4.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@lishaduck lishaduck changed the base branch from main to major October 26, 2025 01:38
@aklinker1
Copy link
Member

aklinker1 commented Oct 27, 2025

Ugh, this diff is massive, did you rebase it correctly?

@lishaduck
Copy link
Contributor Author

lishaduck commented Oct 27, 2025

Ugh, this diff is massive, did you rebase it correctly?

Haven't rebased it yet

@lishaduck lishaduck force-pushed the cleanup-esm branch 2 times, most recently from c603e7d to 8fab501 Compare October 27, 2025 22:36
aklinker1
aklinker1 requested changes Oct 27, 2025
View reviewed changes
Copy link
Member

@aklinker1 aklinker1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We can also cleanup this file: https://github.com/wxt-dev/wxt/blob/main/packages/wxt/src/core/utils/eslint.ts

Other than the changes that make Vite a required dependency, the rest looks good.

Also, should this be based on the main branch? Or did you intend this to be a breaking change as well?

lishaduck
lishaduck commented Oct 28, 2025
View reviewed changes
Copy link
Contributor Author

@lishaduck lishaduck left a comment

Choose a reason for hiding this comment

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

Oh, apparently I didn't hit submit on my review, sorry for leaving you without guidance.

I was a bit confused, like didn't I just clarify + ask you a followup question? Makes sense.

@lishaduck
Copy link
Contributor Author

lishaduck commented Oct 28, 2025 •
edited
Loading

We can also cleanup this file: main/packages/wxt/src/core/utils/eslint.ts

Not sure what you mean, I did, right? eslint is an optional peerDependency, so it still needs the dynamic import.

Other than the changes that make Vite a required dependency, the rest looks good.

Vite is already a required dependency, is still a required dependency following #1945, and I'm skeptical of the multiple builders argument (I'll follow up on that above).
If you're referencing non-dynamic imports of Vite, those also already exist. I thought it might be intentional, but I decided it was just for CJS-compat because there are a few (two?) files that just directly (non-type) import 'vite'.

I'll revert the changes, but I'm pretty lost as to the reasoning.

Also, should this be based on the main branch? Or did you intend this to be a breaking change as well?

I didn't want to target changes that might be wrong, but having messed around with the codebase more, I'm pretty confident with them.

@aklinker1
Copy link
Member

aklinker1 commented Oct 28, 2025 •
edited
Loading

The other imports to vite are because I got lazy... I'd prefer to keep it abstracted for now even if it's the only builder WXT supports. Maybe it's about time I clean those other imports up as well...

Undoing the builder abstraction or more tightly coupling Vite to the project is something I will stubbornly refuse to budge on right now, sorry.

@lishaduck
Copy link
Contributor Author

lishaduck commented Oct 29, 2025 •
edited
Loading

The other imports to vite are because I got lazy... I'd prefer to keep it abstracted for now even if it's the only builder WXT supports. Maybe it's about time I clean those other imports up as well...

✅ It looked like more but they were just types. Could we turn on verbatimModuleSyntax now?

Undoing the builder abstraction or more tightly coupling Vite to the project is something I will stubbornly refuse to budge on right now, sorry.

No need to be sorry! I'm just curious why it's built this way, I was surprised because I thought WXT was all-in on Vite. Sorry if it comes off confrontational (and I'll try not to, but knowing me I'll probably manage to complain whenever I touch these files for all of eternity until I decide to add support for some shiny new bundler, please just ignore me)

@lishaduck lishaduck changed the base branch from major to main October 29, 2025 05:27
@lishaduck lishaduck force-pushed the cleanup-esm branch 2 times, most recently from a7b6b8c to 0e5263b Compare October 29, 2025 05:33
@lishaduck lishaduck requested a review from aklinker1 October 29, 2025 05:37
lishaduck
lishaduck commented Oct 29, 2025
View reviewed changes
@aklinker1 aklinker1 mentioned this pull request Dec 16, 2025
Merged
@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 16, 2025

Open in StackBlitz

@wxt-dev/analytics

npm i https://pkg.pr.new/@wxt-dev/analytics@1950

@wxt-dev/auto-icons

npm i https://pkg.pr.new/@wxt-dev/auto-icons@1950

@wxt-dev/browser

npm i https://pkg.pr.new/@wxt-dev/browser@1950

@wxt-dev/i18n

npm i https://pkg.pr.new/@wxt-dev/i18n@1950

@wxt-dev/module-react

npm i https://pkg.pr.new/@wxt-dev/module-react@1950

@wxt-dev/module-solid

npm i https://pkg.pr.new/@wxt-dev/module-solid@1950

@wxt-dev/module-svelte

npm i https://pkg.pr.new/@wxt-dev/module-svelte@1950

@wxt-dev/module-vue

npm i https://pkg.pr.new/@wxt-dev/module-vue@1950

@wxt-dev/runner

npm i https://pkg.pr.new/@wxt-dev/runner@1950

@wxt-dev/storage

npm i https://pkg.pr.new/@wxt-dev/storage@1950

@wxt-dev/unocss

npm i https://pkg.pr.new/@wxt-dev/unocss@1950

@wxt-dev/webextension-polyfill

npm i https://pkg.pr.new/@wxt-dev/webextension-polyfill@1950

wxt

npm i https://pkg.pr.new/wxt@1950

commit: 1f5b48b

@aklinker1
Copy link
Member

aklinker1 commented Dec 16, 2025

Looks like there are two test failures

@lishaduck
chore: cleanup esm
796dd3d 
As far as I could tell, the original reasons are no longer reproducible.
@lishaduck
refactor: use static imports now that wxt is esm-only
8314d96 
Since wxt-dev#848, there's no need to do all this async work.
At least, as far as I could tell...
@lishaduck
chore: changes i made while hunting stuff down
f85a597 
These seem harmless enough I'll leave them in ig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Timeraa Timeraa Awaiting requested review from Timeraa Timeraa is a code owner

@aklinker1 aklinker1 Awaiting requested review from aklinker1 aklinker1 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lishaduck @aklinker1