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(v2): tighten up the TypeScript onboarding #3244

Merged
merged 3 commits into from
Aug 17, 2020

Conversation

orta
Copy link
Contributor

@orta orta commented Aug 8, 2020

Motivation

Improve the TypeScript experience for an out of the box classic template

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I have a new v2 0.61 project, and this builds the template OOTB with no errors (assuming you merge with master's packages/docusaurus-module-type-aliases/src/index.d.ts anyway) - I'll leave notes inline.

@orta orta requested a review from yangshun as a code owner August 8, 2020 15:23
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 8, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Aug 8, 2020

Deploy preview for docusaurus-2 ready!

Built with commit cb37c7a

https://deploy-preview-3244--docusaurus-2.netlify.app

@@ -53,6 +53,28 @@ declare module '@theme-original/*';

declare module '@docusaurus/*';
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 globally applies an any to all @docusaurus/* stuff, but because the following are more specific, then they get picked up as the module definition. This means as you add new things to the module TS users just get any, but then it can get filled in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2020-08-08 at 11 26 02 AM

declare module '@docusaurus/useBaseUrl' {
export default function(relativePath: string): string
}

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 use the import("x") syntax, which means recommending people add the @types/x for them ahead of time

website/docs/typescript-support.md Show resolved Hide resolved
"noEmit": true,
"noImplicitAny": false
},
"extends": "@tsconfig/docusaurus/tsconfig.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moves all these config options to tsconfig/bases#27 - then it's obvious what is framework vs project

```ts title="src/types.d.ts"
/// <reference types="@docusaurus/module-type-aliases" />
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed with https://github.com/tsconfig/bases/pull/28/files

This has one side-effect of encouraging people to be explicit in their global types, so for example if they wanted to add Jest tests, it would need to look like:

{
  "extends": "@tsconfig/docusaurus",
  "types": ["node",  "jest", "@docusaurus/module-type-aliases"],
  "include": ["src"]
}

But I imagine that this is the minority of folks, so I've left it off

```

This file makes TypeScript recognize various Docusaurus specific webpack aliases like `@theme`, `@docusaurus`, `@generated`.
Docusaurus doesn't use this `tsconfig.json` to compile your project. It is added just for a nicer Editor experience, although you can choose to run `tsc` to type check your code for yourself or on CI.
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 config specifies noEmit - so no need to include it here

@orta
Copy link
Contributor Author

orta commented Aug 8, 2020

Ah, good, TypeScript runs across this repo too - that should help make this more accurate.

@orta
Copy link
Contributor Author

orta commented Aug 9, 2020

This ended up a bit bigger than I had anticipated, but as I tightened the types for consumers it also tightened the types used internally - will comment

@@ -127,4 +127,4 @@ class PendingNavigation extends React.Component<Props, State> {
}
}

export default withRouter(PendingNavigation);
export default withRouter(PendingNavigation as any) as any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

introducing @types/react-router-dom gave a fail here because the objects don't all match up - this is a known issue in the types for withRouter - it expects to be used as a decorator. I tried using a disable next error for this, but it still propagated into a bunch of places, so I just as any'd it

It previously was an any

@@ -142,7 +142,7 @@ function Link({
{...props}
onMouseEnter={onMouseEnter}
innerRef={handleRef}
to={targetLink}
to={targetLink || ''}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to has always been a string | undefined and if it goes through the undefined path then it ends up a <a and not down this branch, as the linter doesn't allow a ! then I opted for the 'should never occur' || ""

@@ -5,6 +5,7 @@
"lib": ["es2017","es2019.array", "DOM"],
"declaration": true,
"declarationMap": true,
"jsx": "react",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

optional and doesn't affect the output to my knowledge, but I get errors everywhere in vscode without it

@slorber
Copy link
Collaborator

slorber commented Aug 11, 2020

Thanks for this, we definitively need to improve the TS story :)

Quite busy this week but will check it in detail at the end of the week.

Also think it would be nice if we migrated our own website to TS to dogfood

@slorber
Copy link
Collaborator

slorber commented Aug 17, 2020

thanks :)

@slorber slorber changed the title Tighten up the TypeScript onboarding feat(v2): tighten up the TypeScript onboarding Aug 17, 2020
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Aug 17, 2020
@slorber slorber changed the title feat(v2): tighten up the TypeScript onboarding chore(v2): tighten up the TypeScript onboarding Aug 17, 2020
@slorber slorber added pr: maintenance This PR does not produce any behavior differences to end users when upgrading. pr: polish This PR adds a very minor behavior improvement that users will enjoy. and removed pr: bug fix This PR fixes a bug in a past release. labels Aug 17, 2020
@slorber slorber merged commit 33ecc4b into facebook:master Aug 17, 2020
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: maintenance This PR does not produce any behavior differences to end users when upgrading. pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants