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

fix(v2): fix bad @docusaurus/types Plugin type generics #5058

Merged
merged 3 commits into from
Jun 25, 2021

Conversation

jsamr
Copy link
Contributor

@jsamr jsamr commented Jun 24, 2021

Motivation

When building a Typescript project depending on @docusaurus/types, this error is raised:

type-no-generic

type-no-generic

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

I have added a tsconfig.json file to @docusaurus/types and typescript as a dev dep. One can check those types now with

yarn workspace @docusaurus/types test

The test now passes:

test-passes

`LoadedPlugin` referenced line 201 is not generic, causing typescript errors on
end-user builds.
Contributors will no longer inadvertently dump type errors since any IDE should
check types now.
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 24, 2021
@netlify
Copy link

netlify bot commented Jun 24, 2021

✔️ [V2]

🔨 Explore the source changes: 9c08bb6

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60d5928573140700075e62e9

😎 Browse the preview: https://deploy-preview-5058--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Jun 24, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 70
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5058--docusaurus-2.netlify.app/

@@ -13,11 +13,17 @@
"directory": "packages/docusaurus-types"
},
"license": "MIT",
"scripts": {
"test": "tsc -p ."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to catch the error? Because I tried locally and it doesn't.

Actually trying to find a solution to catch such mistakes 🤪 let me know if you find one

Copy link
Contributor Author

@jsamr jsamr Jun 25, 2021

Choose a reason for hiding this comment

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

Oh I see what happened. I tested a custom tsconfig.json file, then I realized there was a shared config and used extends. But the TS server didn't reboot and kept the former config, thus I thought the new config would still catch those errors.

And the new config brought "skipLibCheck": true which caused declaration files to be ignored. But if I revert that, all those @types/webpack error spawn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those webpack errors come from conflicting tapable definitions. @types/webpack@4.41.29 depends on @types/tapable@^1" while webpack@5.40.0depends ontapable@^2` which provides its own definitions.

Not sure what is meant by "@types/webpack and webpack/types.d.ts are not the same thing" 😓 (in root tsconfig); but either upgrading @types/webpack to ^5 or sticking with TS types provided by webpack might fix the issue...

Those TS errors in node_modules are maddening!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Previously Webpack used definitively typed (4.x); but it started to ship official typedefs in 5.x. The initial 5.x typedefs were missing a lot of things, I think they completed support more recently so we can probably try again to remove @types/webpack once for all

@@ -283,12 +283,14 @@ export interface Plugin<Content> {
}): ThemeConfig;
}

export type InitializedPlugin = Plugin<unknown> & {
export type InitializedPlugin<Content = unknown> = Plugin<Content> & {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also adding the missing plugin content generic to these derived types

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jun 25, 2021
@slorber slorber changed the title Fix @docusaurus/types tsc build error fix(v2): fix bad @docusaurus/types Plugin type generics Jun 25, 2021
@slorber slorber merged commit 0ae8a09 into facebook:master Jun 25, 2021
@jsamr jsamr deleted the fix-types branch June 25, 2021 13:19
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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants