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

Upgrade Jest website to Docusaurus v3 #14463

Merged
merged 17 commits into from
Sep 7, 2023
Merged

Conversation

slorber
Copy link
Contributor

@slorber slorber commented Aug 31, 2023

@netlify
Copy link

netlify bot commented Aug 31, 2023

Deploy Preview for jestjs ready!

Name Link
🔨 Latest commit e393b3a
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/64f9db90cb045f0008e91706
😎 Deploy Preview https://deploy-preview-14463--jestjs.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.

website/package.json Outdated Show resolved Hide resolved
Copy link
Contributor Author

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Ready to review 🎉

The visual diff reported by Argos between prod and PR looks minimal and mostly false positives
(sometimes it's very subtle and related to font anti-aliasing rendering)

https://app.argos-ci.com/slorber/jest-website-visual-tests

Note, one of the diffs reported is related to a typo that will be fixed by another PR: #14466

README.md Outdated
Comment on lines 156 to 157
<details><summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
<details>
<summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In MDX v2 this does not compile.
This is the readme though so not a big deal but I updated it too because mdx docs have the exact same content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

Comment on lines 246 to 247
If you are seeing coverage output such as...

If you are seeing coverage output such as...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In MDX v2 not having a line break will wrap the summary in an extra useless paragraph. This leads browsers to ignore the nested summary tag and display the default label instead of the provided one.

CF problem reported by Argos here: https://app.argos-ci.com/slorber/jest-website-visual-tests/builds/11/56882867

CleanShot 2023-09-01 at 16 45 20@2x CleanShot 2023-09-01 at 16 45 32@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

docs/Configuration.md Outdated Show resolved Hide resolved
Comment on lines 92 to 93
<details><summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
<details>
<summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In MDX v2 this does not compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

:::warning
:::danger
Copy link
Contributor Author

Choose a reason for hiding this comment

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

:::warning is a legacy Docusaurus admonition alias that is not documented, see https://docusaurus.io/docs/markdown-features/admonitions

Issue reported by Argos due to default label change: https://app.argos-ci.com/slorber/jest-website-visual-tests/builds/11/56882879

CleanShot 2023-09-01 at 16 48 20@2x CleanShot 2023-09-01 at 16 48 44@2x

Using :::danger is the official red admonition name, keep the same label as before


Note: we are going to reintroduce :::warning soon, but it will be yellow instead of red (facebook/docusaurus#7558)

Copy link
Member

Choose a reason for hiding this comment

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

I think we wanted this to be yellow. That doesn't currently exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The yellow admonition is currently :::caution.

I'd suggest to change that in another PR: the goal of this PR is to focus on having no regression and not improving things ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

Choose a reason for hiding this comment

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

yellow is better!!

Copy link
Member

Choose a reason for hiding this comment

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

landed that yesterday #14493

@slorber slorber marked this pull request as ready for review September 1, 2023 14:53
@slorber slorber changed the title WIP - Docusaurus v3 upgrade Docusaurus v3 upgrade Sep 1, 2023
@slorber slorber changed the title Docusaurus v3 upgrade Upgrade Jest website to Docusaurus v3 Sep 1, 2023
Comment on lines -1756 to 1758
The path to a module that can resolve test<->snapshot path. This config option lets you customize where Jest stores snapshot files on disk.
The path to a module that can resolve test\<->snapshot path. This config option lets you customize where Jest stores snapshot files on disk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In MDX v2 this does not compile. Escaping is a good solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB this can't be extracted as is to a docs prep PR because on MDX v1, \ will stay visible. The only solution that works on MDX v1 + v2 is test&lt;->snapshot which IMHO is much less maintainable compared to \<->.

For this reason I think it's better to keep this change in this PR

@SimenB
Copy link
Member

SimenB commented Sep 1, 2023

The changes for mdx 2 - can they land separately, or do they not work for v1? Just so we reduce this PR down as much as possible down to just the version change

yarn.lock Outdated
@@ -5269,6 +5808,17 @@ __metadata:
languageName: node
linkType: hard

"@types/react@npm:>=16, @types/react@npm:^18.2.21":
Copy link
Member

Choose a reason for hiding this comment

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

Seems this is causing issues. Would a dedupe help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran yarn dedupe but it doesn't seem to fix CI failures.

I don't really know what's the problem here 😅 any idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to fix it, seems to work but dedupe actually creates problems 😅

@slorber
Copy link
Contributor Author

slorber commented Sep 1, 2023

The changes for mdx 2 - can they land separately, or do they not work for v1? Just so we reduce this PR down as much as possible down to just the version change

I can do that, this is what I did for React-Native (facebook/react-native-website#3780) with 2 "preparation PRs" because there were a lot more content changes to make compared to Jest.

do they not work for v1?

Unfortunately there's a tradeoff, and indeed adapting the content to make it work for both MDX v1 and MDX v2 is not always the "ideal solution".

For example: The path to a module that can resolve test\<->snapshot path.

Under MDX v1: the \n excape char will be displayed, but not in v2.

If you want something that works with both versions, you would have to use a less ideal form: test&lt;->snapshot

So, it's up to you if we should either:

  • do all the content preparation in upfront PRs, with a less than ideal shared syntax
  • do most content preparation in upfront PRs, and keep some content changes like the one above for the v3 upgrade PR

Copy link
Contributor Author

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Looks like I was able to make all the tests pass.

constraints.pro Show resolved Hide resolved
@@ -21,6 +21,7 @@
"@types/babel__template": "^7.0.2",
"@types/node": "~14.14.45",
"@types/which": "^3.0.0",
"@types/ws": "8.5.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the newer 8.5.5 is causing failures due to a breaking change (a type became non-generic while it was before)

@@ -172,6 +173,7 @@
},
"resolutions": {
"@types/node": "~14.14.45",
"@types/react": "^18.2.21",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only way I was able to avoid TS issues. Other alternatives can be explored (reduce hoisting etc) but that looks fine to me?

Comment on lines -772 to +776
React.createElement(Consumer, null, () =>
React.createElement('div', null, 'child'),
),
React.createElement(Consumer, {
children: () => React.createElement('div', null, 'child'),
}),
),
).toBe('<Context.Consumer>\n [Function anonymous]\n</Context.Consumer>');
).toBe('<Context.Consumer>\n [Function children]\n</Context.Consumer>');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With React 18 it seems ReactNode (3rd arg) doesn't accept a function anymore so I replaced it with props.children

@SimenB
Copy link
Member

SimenB commented Sep 5, 2023

do most content preparation in upfront PRs, and keep some content changes like the one above for the v3 upgrade PR

Yeah, that seems ideal 👍

I'll take a look at the React types stuff, but your solution might be the right one

Copy link
Contributor Author

@slorber slorber left a comment

Choose a reason for hiding this comment

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

do most content preparation in upfront PRs, and keep some content changes like the one above for the v3 upgrade PR

Yeah, that seems ideal 👍

Note there are only 3 docs changes, of which only 2 can be moved to a docs preparation PR without bad tradeoffs

CleanShot 2023-09-05 at 11 56 58@2x

All the other docs changes are exactly the same changes, and related to versioned docs.

Do you still want me to extract these 2 docs changes (+ versioned docs) in a separate preparation PR?

Comment on lines -1756 to 1758
The path to a module that can resolve test<->snapshot path. This config option lets you customize where Jest stores snapshot files on disk.
The path to a module that can resolve test\<->snapshot path. This config option lets you customize where Jest stores snapshot files on disk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB this can't be extracted as is to a docs prep PR because on MDX v1, \ will stay visible. The only solution that works on MDX v1 + v2 is test&lt;->snapshot which IMHO is much less maintainable compared to \<->.

For this reason I think it's better to keep this change in this PR

README.md Outdated
Comment on lines 156 to 157
<details><summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
<details>
<summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

Comment on lines 246 to 247
If you are seeing coverage output such as...

If you are seeing coverage output such as...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

Comment on lines 92 to 93
<details><summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
<details>
<summary markdown="span"><strong>Making your Babel config jest-aware</strong></summary>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

:::warning
:::danger
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change can be moved to a preparation PR and would work with both MDX v1 and v2

@SimenB
Copy link
Member

SimenB commented Sep 5, 2023

I'll take a look at the React types stuff, but your solution might be the right one

Ended up with #14470, so the approach in this PR seems fine for now as that needs to wait for Jest 30.

@SimenB
Copy link
Member

SimenB commented Sep 5, 2023

Do you still want me to extract these 2 docs changes (+ versioned docs) in a separate preparation PR?

Yeah, that'd be great, thanks!

"react-github-btn": "^1.3.0",
"react-lite-youtube-embed": "^2.2.2",
"react-markdown": "^8.0.0"
},
"devDependencies": {
"@babel/core": "^7.11.6",
"@crowdin/cli": "^3.5.2",
"@docusaurus/module-type-aliases": "^2.0.0",
"@docusaurus/module-type-aliases": "0.0.0-5658",
"@tsconfig/docusaurus": "^1.0.5",
Copy link
Member

Choose a reason for hiding this comment

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

maybe bump this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? bump to what value?

Copy link
Member

Choose a reason for hiding this comment

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

v2 has been released

Copy link
Contributor Author

Choose a reason for hiding this comment

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

??? but it's a Docusaurus dependency, we want it to be v3 (canary) like the other docusaurus dependencies.

All official Docusaurus deps should use the exact same versions.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh! thought you were talking about the module above.

Actually you are right I forgot something, we are using a new official package @docusaurus/tsconfig now

Copy link
Member

Choose a reason for hiding this comment

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

just remove it based on tsconfig/bases#196?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh! thought you were talking about the module above.

Actually you are right I forgot something, we are using a new official package @docusaurus/tsconfig now

👍

@slorber
Copy link
Contributor Author

slorber commented Sep 7, 2023

Let me know if it looks good to you.

It seems the tests are failing on main now so not sure if this PR is ok or introduces new failures.

It's probably due to Node 20.6 that apparently breaks a few things in the ecosystem.


However this test is failing too: https://github.com/jestjs/jest/actions/runs/6110952563/job/16585434858?pr=14463

Maybe this timeout is expected and it's a flaky test?

CleanShot 2023-09-07 at 17 27 09@2x

@slorber slorber requested a review from SimenB September 7, 2023 15:25
@SimenB
Copy link
Member

SimenB commented Sep 7, 2023

Just flake, yeah. And node 20 is broken due to nodejs/node#49497

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

looks great from I can tell from clicking around in the preview. Thanks for doing this!

@SimenB SimenB merged commit 9171085 into jestjs:main Sep 7, 2023
76 of 89 checks passed
@SimenB
Copy link
Member

SimenB commented Sep 7, 2023

image

bless! 😀

@SimenB
Copy link
Member

SimenB commented Sep 7, 2023

hah #14496

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants