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): Support swizzling TypeScript components #2671

Merged
merged 9 commits into from
Jun 25, 2020
Merged

feat(v2): Support swizzling TypeScript components #2671

merged 9 commits into from
Jun 25, 2020

Conversation

SamChou19815
Copy link
Contributor

@SamChou19815 SamChou19815 commented Apr 27, 2020

Motivation

In #2470, I discussed my plan to implement support for TypeScript theme components. In the original issue theme, one concern of a maintainer is that people might not want TypeScript components. To address that concern, we must maintain the ability to emit JavaScript code when swizzling.

My proposal is to use babel to strip away type annotations in TypeScript components, and make swizzle still emit JS by default, but make swizzle emit TS when --typescript flag has been passed. This pull request implements this proposal.

Implementation Notes

Generating JS theme components from TS theme components

I did a proof-of-concept implementation in #2614. I found that the JS code emitted by Babel doesn't respect the original code style. There is an easy fix: we run prettier on those babel-generated files, so that we get back our original style.

Swizzling Strategy

To support swizzling TS components, an optional getTypeScriptThemePath() method is added to the plugin API. It's not used for module resolution, but purely for swizzling. If --typescript flag is added, then swizzling will read from getTypeScriptThemePath() instead and fallback to getThemePath() if getTypeScriptThemePath() is undefined.

Type Checking

Since JS code is generated by Babel, the TS theme code won't be type checked. Currently, there are some type errors in the code, but as you can see it doesn't block you from launching the website. I will fix these errors once the maintainer thinks the general strategy is good.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

  • Run yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic --typescript, you will see the entire packages/docusaurus-theme-classic/src/theme folder is copied to website/src/theme.
  • Run yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic, you will see the entire packages/docusaurus-theme-classic/src/theme folder is copied to website/src/theme, except that all the TS files are replaced by their corresponding JS code.
  • Run yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic NotFound --typescript, you will see a website/src/theme/NotFound.tsx
  • Run yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic NotFound, you will see a website/src/theme/NotFound.js
  • Run yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic TabItem --typescript, you will see a website/src/theme/TabItem/index.tsx
  • Run yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic TabItem, you will see a website/src/theme/TabItem/index.js

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

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

Deploy preview for docusaurus-2 ready!

Built with commit 6d495db

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

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Apr 27, 2020

Deploy preview for docusaurus-2 ready!

Built with commit d6c3a19

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

Copy link
Contributor

@fanny fanny left a comment

Choose a reason for hiding this comment

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

You don't made any breaking change, your implementation looks good to me, thank you @SamChou19815!

@SamChou19815
Copy link
Contributor Author

@yangshun @lex111 @slorber I updated the implementation to make swizzle TS components rely on less hacky methods. I would like to get your thoughts before fixing the last few devx issues.

@slorber
Copy link
Collaborator

slorber commented Jun 22, 2020

Hey,

That's really great, I really hope to work in theme with TS soon :)

Also had this similar idea to use Babel, but thought about using babel at swizzle time without reusing the /lib js output.

Was wondering, is the babel compiled code of /lib readable enough?
Because as they'll serve as source file for the user, it should be easy to modify them.

For example, it can be annoying if the swizzled file has common-js and Object.assign() instead ES imports/exports and spread? (didn't try in theme classic, but it seems to be the case in other plugins)

According to the webpack config, all our packages + swizzled comps will be transpiled anyway

export function excludeJS(modulePath: string): boolean {
  // always transpile client dir
  if (modulePath.startsWith(clientDir)) {
    return false;
  }
  // Don't transpile node_modules except any docusaurus npm package
  return (
    /node_modules/.test(modulePath) &&
    !/(docusaurus)((?!node_modules).)*\.jsx?$/.test(modulePath)
  );
}

Maybe we could try to see if our TS compilation process (via tsc) can output more modern ES code? Keeping newest ES features instead of downgrading them.

The idea for me would be that swizzling a theme classic comp today should give the same output as tomorrow when theme classic will be written in TS (or quite similar).

@slorber
Copy link
Collaborator

slorber commented Jun 22, 2020

Didn't find much informations on the advantages of tsc vs babel ts preset, but I think we should rather stick to tsc for now, just to keep the setup uniform, and maybe migrate later.

Also I think things like incremental compilation works better in tsc. But maybe we have more options to control the output JS with babel?

Hard to find quality information on this...

@SamChou19815
Copy link
Contributor Author

@slorber

Maybe we could try to see if our TS compilation process (via tsc) can output more modern ES code? Keeping newest ES features instead of downgrading them.

The idea for me would be that swizzling a theme classic comp today should give the same output as tomorrow when theme classic will be written in TS (or quite similar).

TS can be configured to produce nice ES code, but the biggest blockers is that it doesn't preserve empty lines. See microsoft/TypeScript#843 (And you can see a real example in the compiled packages/docusaurus/lib/client/exports/ComponentCreator.js)

Also had this similar idea to use Babel, but thought about using babel at swizzle time without reusing the /lib js output.

It means we need to bundle babel, which might not be a big problem since we already included babel in docusaurus core. But to have a more readable code I also automatically run prettier after babel, then we also need to bundle prettier...

Was wondering, is the babel compiled code of /lib readable enough?

Yes, here is an example of the compiled CodeBlock

/**
 * Copyright (c) Facebook, Inc. and its affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

/* eslint-disable jsx-a11y/no-noninteractive-tabindex */
import React, {useEffect, useState, useRef} from 'react';
import clsx from 'clsx';
import Highlight, {defaultProps} from 'prism-react-renderer';
import copy from 'copy-text-to-clipboard';
import rangeParser from 'parse-numeric-range';
import useDocusaurusContext from '@docusaurus/useDocusaurusContext';
import usePrismTheme from '@theme/hooks/usePrismTheme';
import styles from './styles.module.css';
const highlightLinesRangeRegex = /{([\d,-]+)}/;

const getHighlightDirectiveRegex = (
  languages = ['js', 'jsBlock', 'jsx', 'python', 'html'],
) => {
  // supported types of comments
  const comments = {
    js: {
      start: '\\/\\/',
      end: '',
    },
    jsBlock: {
      start: '\\/\\*',
      end: '\\*\\/',
    },
    jsx: {
      start: '\\{\\s*\\/\\*',
      end: '\\*\\/\\s*\\}',
    },
    python: {
      start: '#',
      end: '',
    },
    html: {
      start: '<!--',
      end: '-->',
    },
  }; // supported directives

  const directives = [
    'highlight-next-line',
    'highlight-start',
    'highlight-end',
  ].join('|'); // to be more reliable, the opening and closing comment must match

  const commentPattern = languages
    .map(
      (lang) =>
        `(?:${comments[lang].start}\\s*(${directives})\\s*${comments[lang].end})`,
    )
    .join('|'); // white space is allowed, but otherwise it should be on it's own line

  return new RegExp(`^\\s*(?:${commentPattern})\\s*$`);
}; // select comment styles based on language

const highlightDirectiveRegex = (lang) => {
  switch (lang) {
    case 'js':
    case 'javascript':
    case 'ts':
    case 'typescript':
      return getHighlightDirectiveRegex(['js', 'jsBlock']);

    case 'jsx':
    case 'tsx':
      return getHighlightDirectiveRegex(['js', 'jsBlock', 'jsx']);

    case 'html':
      return getHighlightDirectiveRegex(['js', 'jsBlock', 'html']);

    case 'python':
    case 'py':
      return getHighlightDirectiveRegex(['python']);

    default:
      // all comment types
      return getHighlightDirectiveRegex();
  }
};

const codeBlockTitleRegex = /title=".*"/;
export default ({children, className: languageClassName, metastring}) => {
  const {
    siteConfig: {
      themeConfig: {prism = {}},
    },
  } = useDocusaurusContext();
  const [showCopied, setShowCopied] = useState(false);
  const [mounted, setMounted] = useState(false); // The Prism theme on SSR is always the default theme but the site theme
  // can be in a different mode. React hydration doesn't update DOM styles
  // that come from SSR. Hence force a re-render after mounting to apply the
  // current relevant styles. There will be a flash seen of the original
  // styles seen using this current approach but that's probably ok. Fixing
  // the flash will require changing the theming approach and is not worth it
  // at this point.

  useEffect(() => {
    setMounted(true);
  }, []);
  const button = useRef(null);
  let highlightLines = [];
  let codeBlockTitle = '';
  const prismTheme = usePrismTheme();

  if (metastring && highlightLinesRangeRegex.test(metastring)) {
    const highlightLinesRange = metastring.match(highlightLinesRangeRegex)[1];
    highlightLines = rangeParser
      .parse(highlightLinesRange)
      .filter((n) => n > 0);
  }

  if (metastring && codeBlockTitleRegex.test(metastring)) {
    codeBlockTitle = metastring
      .match(codeBlockTitleRegex)[0]
      .split('title=')[1]
      .replace(/"+/g, '');
  }

  let language =
    languageClassName && languageClassName.replace(/language-/, '');

  if (!language && prism.defaultLanguage) {
    language = prism.defaultLanguage;
  } // only declaration OR directive highlight can be used for a block

  let code = children.replace(/\n$/, '');

  if (highlightLines.length === 0 && language !== undefined) {
    let range = '';
    const directiveRegex = highlightDirectiveRegex(language); // go through line by line

    const lines = children.replace(/\n$/, '').split('\n');
    let blockStart; // loop through lines

    for (let index = 0; index < lines.length; ) {
      const line = lines[index]; // adjust for 0-index

      const lineNumber = index + 1;
      const match = line.match(directiveRegex);

      if (match !== null) {
        const directive = match
          .slice(1)
          .reduce((final, item) => final || item, undefined);

        switch (directive) {
          case 'highlight-next-line':
            range += `${lineNumber},`;
            break;

          case 'highlight-start':
            blockStart = lineNumber;
            break;

          case 'highlight-end':
            range += `${blockStart}-${lineNumber - 1},`;
            break;

          default:
            break;
        }

        lines.splice(index, 1);
      } else {
        // lines without directives are unchanged
        index += 1;
      }
    }

    highlightLines = rangeParser.parse(range);
    code = lines.join('\n');
  }

  const handleCopyCode = () => {
    copy(code);
    setShowCopied(true);
    setTimeout(() => setShowCopied(false), 2000);
  };

  return (
    <Highlight
      {...defaultProps}
      key={String(mounted)}
      theme={prismTheme}
      code={code}
      language={language}>
      {({className, style, tokens, getLineProps, getTokenProps}) => (
        <>
          {codeBlockTitle && (
            <div style={style} className={styles.codeBlockTitle}>
              {codeBlockTitle}
            </div>
          )}
          <div className={styles.codeBlockContent}>
            <button
              ref={button}
              type="button"
              aria-label="Copy code to clipboard"
              className={clsx(styles.copyButton, {
                [styles.copyButtonWithTitle]: codeBlockTitle,
              })}
              onClick={handleCopyCode}>
              {showCopied ? 'Copied' : 'Copy'}
            </button>
            <div
              tabIndex={0}
              className={clsx(className, styles.codeBlock, {
                [styles.codeBlockWithTitle]: codeBlockTitle,
              })}>
              <div className={styles.codeBlockLines} style={style}>
                {tokens.map((line, i) => {
                  if (line.length === 1 && line[0].content === '') {
                    line[0].content = '\n'; // eslint-disable-line no-param-reassign
                  }

                  const lineProps = getLineProps({
                    line,
                    key: i,
                  });

                  if (highlightLines.includes(i + 1)) {
                    lineProps.className = `${lineProps.className} docusaurus-highlight-code-line`;
                  }

                  return (
                    <div key={i} {...lineProps}>
                      {line.map((token, key) => (
                        <span
                          key={key}
                          {...getTokenProps({
                            token,
                            key,
                          })}
                        />
                      ))}
                    </div>
                  );
                })}
              </div>
            </div>
          </div>
        </>
      )}
    </Highlight>
  );
};

@slorber
Copy link
Collaborator

slorber commented Jun 22, 2020

Thanks, that looks readable :)

Asked the question on twitter and getting some interesting answers: https://twitter.com/sebastienlorber/status/1275067106254393347

Seems like Babel might be faster to build and provide more flexibility on output. Some recommend TSC but without strong arguments apart being more easy to setup (not sure I agree anyway).

It means we need to bundle babel

You mean bundle in the swizzle cli? I'm not sure it's a big deal but if we can avoid that's not bad either.


What bothers me is that we use tsc on all others projects and that this one will be the only one using Babel. I really like babel + TSC, but when I use it I use it everywhere, and also run tsc --noEmit on the side for typechecking.

Can this serve as a poc, eventually leading to migrating other projects from tsc to babel?

Or are we going to stay with tsc + babel?


Also something to consider: do we need to emit typedefs for the theme?

What's the story, when a TS user wants to override a theme component, importing original theme components with the aliases?

Like import Footer from '@theme-original/Footer'

@SamChou19815
Copy link
Contributor Author

What bothers me is that we use tsc on all others projects and that this one will be the only one using Babel. I really like babel + TSC, but when I use it I use it everywhere, and also run tsc --noEmit on the side for typechecking.

I do plan to add tsc --noEmit once I finished everything. It's not added for now because things don't type check at the moment.

Also something to consider: do we need to emit typedefs for the theme?

This is on my future plan. Actually that's one of the reasons why I'm very passionate about this PR.

Now everything can pass the type checker! (although still a lot of any)
@SamChou19815
Copy link
Contributor Author

Everything passes the type checker. I think it's good for review now.

Copy link
Collaborator

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

Some TS things can be simplified (subjective so I'll let you decide), but overall LGTM

@@ -7,6 +7,11 @@
"access": "public"
},
"license": "MIT",
"scripts": {
"tsc": "tsc --noEmit && yarn babel && yarn prettier",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably rename the tsc scripts to just build or something (can be in another PR)

Also, I added recently a global yarn watch command, all TS subprojects should rather implement it as well

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx Outdated Show resolved Hide resolved
packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx Outdated Show resolved Hide resolved
let fromPath = pluginInstance.getThemePath?.();
let fromPath = typescript
? (pluginInstance.getTypeScriptThemePath ?? pluginInstance.getThemePath)?.()
: pluginInstance.getThemePath?.();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should fail fast here

I tried the following and expected it to fail: yarn workspace docusaurus-2-website swizzle @docusaurus/theme-live-codeblock Playground --typescript

but instead it did swizzle the components in JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to avoid fallback to getThemePath

@slorber
Copy link
Collaborator

slorber commented Jun 23, 2020

Hey @SamChou19815 , the classic theme conversion to TS looks great to me and seems ready to merge (just did a few subjective TS suggestions).

About the swizzling experience, I tried to see what would be the experience of an user trying to use TS on his docusaurus site, so I tried: yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic Footer --typescript

The swizzling did work fine but the experience wasn't so great, because I ended up with IDE errors in D2 website project 🤪

image


What do you think about splitting your work in multiple PRs to make it simpler to review/merge?

  • convert theme to TS (this part seems ready to merge)
  • ability to easily swizzle ts + dogfood some TS on D2 website + try to import things like '@theme-original/Footer'
  • support TS in docusaurus-init so that it's easy to start a D2 TS-based website?

@SamChou19815
Copy link
Contributor Author

The swizzling did work fine but the experience wasn't so great, because I ended up with IDE errors in D2 website project

It turns out it's very easy to fix. I just added tsconfig.json and types.d.ts that refers to the @docusaurus/module-type-aliases to unbreak IDE experience. Let me know if it's good.

Copy link
Collaborator

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

That looks good to me.

Just I tried to create this swizzle:

yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic Footer --typescript

import React from 'react';
import OriginalThemeFooter from '@theme-original/Footer';

function Footer(): JSX.Element | null {
  return (
    <div>
      <OriginalThemeFooter />
    </div>
  );
}

export default Footer;

It worked great apart the theme original import

Just adding the type make it work better:

declare module '@theme/*';
declare module '@theme-original/*'; // => missing

We should also document this because the local TS setup is not so obvious. Didn't know about the /// directives before such as

// eslint-disable-next-line spaced-comment
/// <reference types="@docusaurus/module-type-aliases" />

@SamChou19815
Copy link
Contributor Author

SamChou19815 commented Jun 25, 2020

Added declare module '@theme-original/*';. Not sure what's happening in github actions. The same command can run smoothly yesterday, and circle ci can pass the same installation step.

Moving @babel/cli to the root package.json seems to solve the problem above...

@slorber slorber merged commit 5ccd24c into facebook:master Jun 25, 2020
@SamChou19815 SamChou19815 deleted the swizzle-ts-components branch June 25, 2020 15:32
This pull request was closed.
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.

5 participants