Skip to content
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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

chore: update jiti to v2 #10716

wants to merge 5 commits into from

Conversation

lachieh
Copy link
Contributor

@lachieh lachieh commented Nov 22, 2024

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

In adding an example for @shikijs/rehype, a few community members noticed that it doesn't work with the installed version of jiti

Test Plan

Almost all of the tests pass with the changes to moduleUtils. The only test that does not pass is the one below.

 FAIL  packages/docusaurus-utils/src/__tests__/moduleUtils.test.ts
  ● loadFreshModule › module graph › can load and reload fresh module graph
    expect(received).resolves.toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 0

    @@ -1,8 +1,7 @@
      Object {
        "dependency1": Object {
-         "dep1Export": "dep1 val1",
          "dep1Val": "dep1 val2",
        },
        "dependency2": Object {
          "dep2Val": "dep2 val",
        },

      132 |
      133 |       // Should be able to read the initial module graph
    > 134 |       await expect(entryFile.load()).resolves.toEqual({
          |                                               ^
      135 |         someEntryValue: 'entryVal',
      136 |         dependency1: {
      137 |           dep1Export: 'dep1 val1',

      at Object.toEqual (node_modules/expect/build/index.js:174:22)
      at Object.toEqual (packages/docusaurus-utils/src/__tests__/moduleUtils.test.ts:134:47)

Test links

Deploy preview: https://deploy-preview-10716--docusaurus-2.netlify.app/

Related issues/PRs

comment in #9122

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 22, 2024
Copy link

github-actions bot commented Nov 22, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO Report
/ 🟠 59 🟢 98 🟢 96 🟢 100 Report
/docs/installation 🟠 54 🟢 97 🟢 100 🟢 100 Report
/docs/category/getting-started 🟠 74 🟢 100 🟢 100 🟠 86 Report
/blog 🟠 64 🟢 96 🟢 100 🟠 86 Report
/blog/preparing-your-site-for-docusaurus-v3 🔴 48 🟢 92 🟢 100 🟢 100 Report
/blog/tags/release 🟠 64 🟢 96 🟢 100 🟠 86 Report
/blog/tags 🟠 73 🟢 100 🟢 100 🟠 86 Report

Copy link

socket-security bot commented Nov 23, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

Copy link

netlify bot commented Nov 23, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 2322b1d
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/674606a3874d5800082c2165
😎 Deploy Preview https://deploy-preview-10716--docusaurus-2.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 site configuration.

@lachieh lachieh marked this pull request as ready for review November 23, 2024 05:46
@slorber
Copy link
Collaborator

slorber commented Nov 25, 2024

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

@lachieh
Copy link
Contributor Author

lachieh commented Nov 26, 2024

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 interopDefault feature that was reported broken previously is broken again in jiti >=2.2.0.

If I use [email protected] and I run the tests with yarn node --experimental-vm-modules $(yarn bin jest) then it passes all but 1 test. Unfortunately, the experimental-vm-modules support is what jiti should be circumventing, which is only fixed in [email protected].

The real issue is that if I upgrade to jiti@latest (2.4.0 as of writing) the interopDefault functionality doesn't seem to work correctly and breaks the ● loadFreshModule › module graph › can load and reload fresh module graph test. I fixed most of the interopDefault issues by using the mlly#interopDefault function again on the imported module, but the second test is a little harder to get to the bottom of.

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.

The only test that fails that isn't to do with either the Proxy interopDefault implementation in jiti seems to be this one below, and I'm not sure if it is critical. Either way, it doesn't look like it would be too much work to fix.

@lachieh
Copy link
Contributor Author

lachieh commented Nov 26, 2024

@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?

@slorber
Copy link
Collaborator

slorber commented Nov 29, 2024

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 😅

@lachieh
Copy link
Contributor Author

lachieh commented Nov 30, 2024

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?

@slorber
Copy link
Collaborator

slorber commented Dec 3, 2024

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?

Yes, I guess we move the tests to use snapshots instead of assertions. And when upgrading Jiti, we can just update the snapshots to freeze the behavior expectation. If the behavior changes, but overall the system still seems legit and we can document a bit how users should update their config/sidebar/plugin code, I think it's fine.

If we target v4 we can do whatever breaking change we want, including removing the interop or even dropping CJS support. It's preferable to not annoy the users for no good reason though but the behavior can definitively change and we can consider the current tests do not exist.

@lachieh
Copy link
Contributor Author

lachieh commented Dec 3, 2024

Ok, I'll take a look at doing a separate PR that updates the tests to snapshots and then revisit this one after that is merged. Thanks!

@lachieh lachieh marked this pull request as draft December 3, 2024 14:08
MaXFeeD added a commit to Nernar/nernar.github.io that referenced this pull request Dec 4, 2024
MaXFeeD added a commit to Nernar/nernar.github.io that referenced this pull request Dec 4, 2024
* Testing a new way to interact with park

* Squashed 'api/' content from commit 06e27942d

git-subtree-dir: api
git-subtree-split: 06e27942d1def5348e4c82cd6b5afd8b4e79422d

* Migrating to docusaurus-plugin-typedoc-api

* Added libraries, experimental

- TODO: Check library entry points.

* Squashed 'api/typedoc-plugin/' content from commit b069bc730

git-subtree-dir: api/typedoc-plugin
git-subtree-split: b069bc730f29af206c42684c09cdeb6b39d50f00

* Moved packages/plugin/docusaurus-plugin-typedoc-api to docusaurus-typedoc-plugin

* Bump @Docusaurus dependencies (3.6.1 -> 3.6.3)

* Removed typedoc-codicon-theme, since it cannot be used anymore

* Properly linting markdown, ignoring images links

* Added plugin building to package

* Compiling plugin with typescript, copying assets

* Removed plugin footer, useless one

* Typedoc 0.26+ uses ReflectionId as identifier (still number, but with exceptions)

* Plugins should use @theme-init instead of @theme

* Added typedoc 0.26+ document icon

* Added Options type export to entry point, using it in docusaurus.config.ts

* Downgrading to Typedoc 0.25.13 until jiti-v2 is coming out to Docusaurus

- See facebook/docusaurus#10716 for details.

* Migrating plugin back too for now

* Moving plugin to @nernar/docusaurus-plugin-typedoc, migrating to Docusaurus 3.5.2

* Added libraries tsconfig stuff

* Moved declarations to new location

* Added libraries packages to API

* Some dirty stuff to make it work
github-actions bot pushed a commit to Nernar/nernar.github.io that referenced this pull request Dec 4, 2024
* Testing a new way to interact with park

* Squashed 'api/' content from commit 06e27942d

git-subtree-dir: api
git-subtree-split: 06e27942d1def5348e4c82cd6b5afd8b4e79422d

* Migrating to docusaurus-plugin-typedoc-api

* Added libraries, experimental

- TODO: Check library entry points.

* Squashed 'api/typedoc-plugin/' content from commit b069bc730

git-subtree-dir: api/typedoc-plugin
git-subtree-split: b069bc730f29af206c42684c09cdeb6b39d50f00

* Moved packages/plugin/docusaurus-plugin-typedoc-api to docusaurus-typedoc-plugin

* Bump @Docusaurus dependencies (3.6.1 -> 3.6.3)

* Removed typedoc-codicon-theme, since it cannot be used anymore

* Properly linting markdown, ignoring images links

* Added plugin building to package

* Compiling plugin with typescript, copying assets

* Removed plugin footer, useless one

* Typedoc 0.26+ uses ReflectionId as identifier (still number, but with exceptions)

* Plugins should use @theme-init instead of @theme

* Added typedoc 0.26+ document icon

* Added Options type export to entry point, using it in docusaurus.config.ts

* Downgrading to Typedoc 0.25.13 until jiti-v2 is coming out to Docusaurus

- See facebook/docusaurus#10716 for details.

* Migrating plugin back too for now

* Moving plugin to @nernar/docusaurus-plugin-typedoc, migrating to Docusaurus 3.5.2

* Added libraries tsconfig stuff

* Moved declarations to new location

* Added libraries packages to API

* Some dirty stuff to make it work
@slorber
Copy link
Collaborator

slorber commented Dec 6, 2024

By the way, Node is looking to unflag TS type stripping support

Looks like we may even be able to remove Jiti in the future. Maybe we should postpone this update and directly aim for native TS support instead?

See also nodejs/typescript#17 (comment)

@lachieh
Copy link
Contributor Author

lachieh commented Dec 6, 2024

I should have time to work on the testing PR and the jiti issue upstream before the end of the month. Are you looking at 4.0 release soon?

@slorber
Copy link
Collaborator

slorber commented Dec 6, 2024

Not that soon, probably early 2025

I'd like to upgrade to React 19 and React Router 7, among other things. Not sure all our peer dependencies are ready for that yet, we'll see soon.

@slorber slorber added this to the 4.0 milestone Dec 6, 2024
@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: breaking change Existing sites may not build successfully in the new version. Description contains more details.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants