-
-
Couldn't load subscription status.
- Fork 408
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.
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: major
Are you sure you want to change the base?
refactor: cleanup esm #1950
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to keep this file as-is, so that vite's not a required dependency. Eventually, I want to add support for more bundlers than just vite, which is why it's a dynamic import here. I'd like to keep the function async so trying to figure out how to make it async again is not a blocker whenever that happens. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So... I shouldn't work on tying WXT more closely to Vite using the Env API? ... was going to work on that next. Can you explain why we'd want more bundlers than Vite? I'd understand if it was a webpack project and we were adding in support for RSPack or Turbopack, but there's not really Vite alternatives out there beyond Rolldown-Vite, which'll just be vanilla Vite pretty soon. |
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this file is also only about importing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even when Vite is a peer, it'll still be installed (either by error [yarn, pnpm sometimes] or by autoinstall [npm, bun, pnpm usually]). |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,13 +3,14 @@ import { createWslRunner } from './wsl'; | |||
| import { createWebExtRunner } from './web-ext'; | ||||
| import { createSafariRunner } from './safari'; | ||||
| import { createManualRunner } from './manual'; | ||||
| import { isWsl } from '../utils/wsl'; | ||||
|
|
||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| import { wxt } from '../wxt'; | ||||
| import isWsl from 'is-wsl'; | ||||
|
|
||||
| export async function createExtensionRunner(): Promise<ExtensionRunner> { | ||||
| if (wxt.config.browser === 'safari') return createSafariRunner(); | ||||
|
|
||||
| if (await isWsl()) return createWslRunner(); | ||||
| if (isWsl) return createWslRunner(); | ||||
| if (wxt.config.runnerConfig.config?.disabled) return createManualRunner(); | ||||
|
|
||||
| return createWebExtRunner(); | ||||
|
|
||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can just delete this file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was having trouble mocking it, but I'll give it another shot |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import isWsl_ from 'is-wsl'; | ||
|
|
||
| /** | ||
| * Returns true when running on WSL or WSL2. | ||
| */ | ||
| export async function isWsl(): Promise<boolean> { | ||
| const { default: isWsl } = await import('is-wsl'); // ESM only, requires dynamic import | ||
| return isWsl; | ||
| return isWsl_; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ export async function registerWxt( | |
|
|
||
| const hooks = createHooks<WxtHooks>(); | ||
| const config = await resolveConfig(inlineConfig, command); | ||
| const builder = await createViteBuilder(config, hooks, () => wxt.server); | ||
| const builder = createViteBuilder(config, hooks, () => wxt.server); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, and here I'm assuming vite exists lol. We should probably add an if statement and dynamic import There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But leave that for a different PR? And I'm still not sure why it matters if Vite exists. It does exist, and acting like it doesn't just feels like premature abstraction, but 🤷🏻♂️ |
||
| const pm = await createWxtPackageManager(config.root); | ||
|
|
||
| wxt = { | ||
|
|
||
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.
We test this by spinning up NPM projects that have eslint as a dependency, so we don't technically need it installed here. I would prefer not to, just so it's not installed as apart of the monorepo.
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.
The reason we needed it is because otherwise TS errors that there's no types for the
import("eslint").I'll try to shim out a
.d.tsinstead.