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

feat(v2): Provide @theme module alias type definitions for theme-classic #3123

Closed
wants to merge 5 commits into from
Closed

feat(v2): Provide @theme module alias type definitions for theme-classic #3123

wants to merge 5 commits into from

Conversation

SamChou19815
Copy link
Contributor

@SamChou19815 SamChou19815 commented Jul 25, 2020

Motivation

Now we finally migrated the theme-classic code to TypeScript, we are finally in a good position to generate high-quality type definitions directly from source.

With this PR, we can finally get good IDE integrations:

Screen Shot 2020-07-24 at 21 32 46

Implementation

I have to admit that the current implementation is quite hacky due to lack of native support from TS. Therefore, I mostly consider this PR as a proof of concept implementation.

TypeScript already has some support for generating a bundled single d.ts file for all modules. However, its result is not directly usable.

For example, this is a snippet of what you get:

declare module "AnnouncementBar/index" {
  function AnnouncementBar(): JSX.Element | null;
  export default AnnouncementBar;
}

Several problems:

  1. We want it to start with @theme/ to match Docusaurus' webpack type alias.
  2. Ideally, we want to drop the /index at the end.
  3. Some emitted modules typedef has import './styles.css'. They cannot be understood by TypeScript/

Fortunately, this is quite easy to detect and I solved that with a hack by doing string substitution:

'declare module "' => 'declare module "@theme/'
'/index' => ''
'import './styles.css'' => ''

The second issue is where we put the generated type definition file. Ideally, we want to use the generated file in theme-classic as well. Therefore, we should reference it inside packages/docusaurus-theme-classic/src/types.d.ts. However, this creates a cyclic dependency: tsc needs this generated file to compile stuff, and to generate this file we need tsc to compile a project that includes it.

My hacky way to break this cyclic dependency is to initial create an empty file, let tsc generate the type definition elsewhere, do some string substitution and then rewrite the file in the correct place. It's safe to start with an empty d.ts, since the catch-all definition declare module '@theme/*'; ensures that we have an any module to start with.

I would like some ts expert feedback on how to make it less hacky, but that's the best way to generate the type definition that I can think of.

The second problem is how we can use the generated type definition on theme-classic itself. Unfortunately, we can't without introduce nasty hack or cyclic dependencies between source and types. The crossed-out text above describes a hacky way to do it which I abandoned now.

Fortunately, I found a much more principled way. We can configure paths in tsconfig.json to make TS translate imports that starts with @theme-classic/ to src/theme. Note that the prefix has to be @theme-classic/ instead of @theme, because otherwise the type of local module will be shadowed by the catch-all declare module '@theme/*'; in @docusaurus/module-type-aliases. Since we in effect changed the import, we need to do something at the transpilation stage to undo the source level change. My solution is to write an inline babel plugin in babel.config.js that transforms @theme-classic/* import back into @theme/* import.

As a proof-of-concept, I changed the @theme imports of BlogPostPage to @theme-classic import. You can see the compiled output to check that it has been correctly transformed back into @theme import.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

CI passes.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 25, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jul 25, 2020

Deploy preview for docusaurus-2 ready!

Built with commit d2c19c7

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

@SamChou19815 SamChou19815 changed the title feat(v2): [RFC] Generate @theme module alias type definitions for theme-classic feat(v2): [RFC] Provide @theme module alias type definitions for theme-classic Jul 25, 2020
@SamChou19815 SamChou19815 marked this pull request as ready for review July 25, 2020 02:22
@SamChou19815 SamChou19815 changed the title feat(v2): [RFC] Provide @theme module alias type definitions for theme-classic feat(v2): Provide @theme module alias type definitions for theme-classic Aug 1, 2020
@SamChou19815
Copy link
Contributor Author

@slorber @yangshun Updated my implementation with a more principled approach that only requires some hacks on the TS type definition output. Would like your thoughts on the proof-of-concept implementation.

@slorber
Copy link
Collaborator

slorber commented Aug 5, 2020

🤪 sorry for the review delay, I have a hard time understanding what's going on in this PR, and still have some other PRs to review first

@SamChou19815 SamChou19815 marked this pull request as draft August 5, 2020 19:49
@SamChou19815
Copy link
Contributor Author

🤪 sorry for the review delay, I have a hard time understanding what's going on in this PR, and still have some other PRs to review first

I'm converting it to draft right now since I found that it will break TypeScript swizzling. I might need more time to think about the best approach.

@SamChou19815
Copy link
Contributor Author

Closing since approach in #3267 should be the way to go.

@SamChou19815 SamChou19815 deleted the generate-theme-typedef branch August 18, 2020 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants