-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
chore: update jiti to v2 #10716
base: main
Are you sure you want to change the base?
chore: update jiti to v2 #10716
Conversation
⚡️ Lighthouse report for the deploy preview of this PR
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
I already tried to upgrade and couldn't make it work 😅 It looks problematic if only one of those exports works while previously both worked: export const dep1Export = "dep1 val1";
export default {dep1Val: "dep1 val2"} Maybe the current version is not correct, but users rely on it and I'm afraid if we change that behavior we'll have to wait v4 |
Yeah, agreed. There were about ~15 tests broken with a straight upgrade but I managed to get it down to 2. It seems like the If I use The real issue is that if I upgrade to I'm going to get a reduced test case to cover these issues in the jiti repo, but if you've got any thoughts, I'd be happy to run them down a little further.
|
@slorber I've been thinking about the current behavior a little more and there is potential to overwrite a property on the default export with a named export: // file1.ts
export const named = 42;
export default {
named: 43
} // file2.ts
const mod = require('./file1.ts');
console.log(mod.named) // 42 It's expected behavior now since it's how jiti@1 works but it feels like something that probably should change in v4. Maybe a full changeover to ESM? |
Honestly, all this feels hard to reason about 🤯 I'm not sure how this interop mode works for the entrypoint, the transitive modules and if this behaves differently for cjs/esm in both case. I think we should ditch the existing test assertions and just rewrite them all with new settings. Like if we implemented this from scratch again. I don't think there's an "ideal interop setup", the tests are mostly here to "freeze" the behavior we have for a given major version, ensuring we keep this behavior. But as far as I understand, users can already alter this behavior with env variables. And it should be fine to change that behavior in major versions. But it will probably require to document the breaking change so we should at least understand a bit how the behavior changed between 2 major versions 😅 |
I've been talking with jiti maintainer over in unjs/jiti#341 about it and it's agreed there is a difference between 2.1 and 2.2 as well as a proposed fix. Are you saying you want to upgrade to jiti@latest and replace the test cases with how the new version behaves for release with docusaurus@4? |
Pre-flight checklist
Motivation
In adding an example for
@shikijs/rehype
, a few community members noticed that it doesn't work with the installed version ofjiti
Test Plan
Almost all of the tests pass with the changes to
moduleUtils
. The only test that does not pass is the one below.Test links
Deploy preview: https://deploy-preview-10716--docusaurus-2.netlify.app/
Related issues/PRs
comment in #9122