-
Notifications
You must be signed in to change notification settings - Fork 63
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: bypass transformation in browser and vitest #158
Conversation
(drafting until we support |
@pi0 Implemented the async |
Seems good start only would you mind separating PR for |
lib/index.mjs
Outdated
typeof document !== "undefined" && | ||
typeof navigator !== "undefined"; | ||
const isVitest = | ||
typeof process !== "undefined" && process.env && process.env.VITEST; |
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 can use std-env
also would isTest
be enough to make implementation generic?
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.
Not sure about other testing env, but generally we do this cause Vitest is based on Vite, let me test a bit with Jest at least. Maybe also for Bun?
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.
Yeah. In general, if we can do it runtime-agnostic even if aiming for vitest for now, i would prefer that. If vite/vitest has special behavior, would be happy to also add specific condition (ideally flag from std-env) -- if we have to.
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.
Jest seems to never pick up the ESM entry, so yeah it should be fine I guess? Updated.
lib/index.mjs
Outdated
}; | ||
|
||
try { | ||
createRequire = await import("module").then((m) => m.createRequire); |
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.
createRequire = await import("module").then((m) => m.createRequire); | |
createRequire = await import("node:module").then((m) => m.createRequire); |
We can also add fallback in .catch
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.
What do you mean by .cache
?
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.
I meaning chaining promise with catch! Will add code refactor.
Co-authored-by: Pooya Parsa <pyapar@gmail.com>
|
||
// In browsers, or in Vitest, | ||
// we bypass jiti transform and fallback to the native import and let the runtime handles it | ||
if (isBrowser || isTest) { |
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 cannot always assume this for browser envs. In an environment like jsdom, isBrowser can be true but also jiti could be working.
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.
Yeah true, but I am not sure if there is a good way to distinguish jsdom and real browser. Any ideas?
I've added new For bun we have a runtime test check that only uses bun native Closing PR for triage and preparing final release but let's discuss how we can make it working out of the box finally for vitest! |
Combining with unjs/unbuild#300, it would allow
unbuild --stub
's output compatible with the browser and Vitest, by opting-out jiti transform on them and forward to Vite's transformation.Playground: https://stackblitz.com/~/github.com/antfu/testing-jiti-on-vite