docusaurus icon indicating copy to clipboard operation
docusaurus copied to clipboard

feat(utils-build): new utility to build plugins

Open Josh-Cena opened this issue 4 years ago • 11 comments

Motivation

Resolve #5564.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Used internally

TODOs

  • [x] Integrate Prettier
  • [x] (After using globs) try not to use internal Babel API
  • [x] Watch mode
  • [ ] Integrate type checks (use tsc in addition to Babel)
    • This is a must, because:
      • CI in the future won't fail even with type errors (can be solved with tsc --noEmit afterwards but better bundle it into the same script;
      • theme-common relies on the output .d.ts file for typing
    • Concerns:
      • Performance: the code is parsed & transformed twice, once by Babel and once by TSC, which is inefficient especially for watch mode
      • The TS compiler API is mysteriously complicated, so maintainability will be low and a lot of the logic from Babel transform can't be reused
    • Related: #2671

Blocking PRs

  • [ ] #5816: I don't want to invest on twice-tranpilation when that PR would make us only transpile once

Josh-Cena avatar Sep 25 '21 06:09 Josh-Cena

✔️ [V2] Built without sensitive environment variables

🔨 Explore the source changes: f8889f5374f8363f9d4cccfa9c4a2872c9143efb

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

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

netlify[bot] avatar Sep 25 '21 07:09 netlify[bot]

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 67
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

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

github-actions[bot] avatar Sep 25 '21 07:09 github-actions[bot]

Not a fan of the package name, we should make it more generic

Any suggestions? I plan to add docusaurus-plugin init and other useful commands in the future, so docusaurus-plugin-builder won't work. docusaurus-plugin-cli doesn't sound right either 🤔

Can we reuse code from babel cli for the watch mode

I've read the cli code and it's really just chokidar + some console messages. I'll try to extract more useful code, but not sure which features we would need

Watch mode does not permit to work on theme with hot reload

I'll check that out. The watch mode is just POC right now

theme / js-theme: unnecessary duplication (main also duplicates, but it's a good opportunity to fix this)

Duplicated in what way? The js-theme only contains the theme components, not the entire src. I feel like deduplicating the .module.css files isn't worth the effort but I can try to figure it out

Build size bot reports important build size changes, any idea why?

My 2 cents is it's just fluctuations, I'll keep an eye on that when I continue working on this

Josh-Cena avatar Sep 29 '21 11:09 Josh-Cena

It looks like the new output is more modern, and it may be a good thing, but it's worth testing a bit on older browsers compared to prod.

image

I don't really like surprising and unexpected changes like that 😅

slorber avatar Sep 29 '21 11:09 slorber

Any suggestions? I plan to add docusaurus-plugin init and other useful commands in the future, so docusaurus-plugin-builder won't work. docusaurus-plugin-cli doesn't sound right either 🤔

@docusaurus/utils-build can still expose a docusaurus-plugin no? One part of this package could be for internal usage, and we only expose some parts publicly

I've read the cli code and it's really just chokidar + some console messages. I'll try to extract more useful code, but not sure which features we would need

Any ref to share? If the babel code doing this is not complex I'm fine, otherwise if it's full of annoying hacks for OSs, shells and FSs, I'd rather keep all this outside of Docusaurus

slorber avatar Sep 29 '21 11:09 slorber

I don't really like surprising and unexpected changes like that 😅

Hmm, wasn't expecting the bundle to change, and it looks like minification has failed somewhere. I'll look into that

Didn't read your inline comments, so ignore some of them. @docusaurus/utils-build sounds fine to me.

See here for babel watch: https://github.com/babel/babel/blob/main/packages/babel-cli/src/babel/dir.ts#L175

Josh-Cena avatar Sep 29 '21 11:09 Josh-Cena

I don't really like surprising and unexpected changes like that 😅

Hmm, wasn't expecting the bundle to change, and it looks like minification has failed somewhere. I'll look into that

Didn't read your inline comments, so ignore some of them. @docusaurus/utils-build sounds fine to me.

See here for babel watch: https://github.com/babel/babel/blob/main/packages/babel-cli/src/babel/dir.ts#L175

Issue is actually in different place, its not related to minification but rather to how babel transforms code.


current build preserves JSX syntax, but transforms imports and rest of the code to cjs than transforming code from jsx to __react.create.... should help with that.

for example:

var _react = _interopRequireDefault(require("react"));
//. ....
  return <article className="margin-vert--lg">
      <_Link.default to={doc.permalink}>
        <h2>{doc.title}</h2>
      </_Link.default>
      {doc.description && <p>{doc.description}</p>}
    </article>;

as for tests and translation issues, currently server requires that files exposed by packages are JSXIdentifier with name Translate witch means that code can't be transformed to JSX and has to be exposed as mjs https://github.com/facebook/docusaurus/blob/284cdabb0a16bf9ec7c581c4852bc5781cfa00f5/packages/docusaurus/src/server/translations/translationsExtractor.ts#L193-L201

<_Link.default href={tag.allTagsPath}>
  <_Translate.default id="theme.tags.tagsPageLink" description="The label of the link targeting the tag list page">
    View All Tags
  </_Translate.default>
</_Link.default>

~in my opinion we should change this code and write babel plugin instead for this~


i do like an idea of package that provide build utils :)

armano2 avatar Nov 16 '21 18:11 armano2

as for tests and translation issues, currently server requires that files exposed by packages are JSXIdentifier with name Translate witch means that code can't be transformed to JSX and has to be exposed as mjs

Technically we should allow to use an alias too, react-intl does it, for inspiration

in my opinion we should change this code and write babel plugin instead for this

Not sure what you have in mind.

I'd like to build a Babel plugin someday so that the translation runtime could self-erase at build time (considering all i18n strings are static), but not sure it's good timing as the frontend ecosystem is migrating to Rust without Babel 🤷‍♂️

What are the advantages of using a plugin for you?

slorber avatar Nov 16 '21 20:11 slorber

What are the advantages of using a plugin for you?

easier/uniform configuration, and this tool can be exposed for 3th party plugin developers

armano2 avatar Nov 17 '21 19:11 armano2

easier/uniform configuration, and this tool can be exposed for 3th party plugin developers

Afaik we don't have any configuration for it and the goal is somehow to hide the fact that it's using Babel under the hood. We should be able to migrate to Rust or whatever without users having to know. Does it make sense?

slorber avatar Nov 18 '21 12:11 slorber

I've recently done a lot of build pipeline refactors that make it really neat and sorted out, even without this unified utility.

  • #7437
  • #7443
  • #7444
  • #7445
  • #7447

The only problem is they are a bit duplicated and scattered. The demand for a unified workflow without copy&pasting the long build/watch scripts still remains.

Josh-Cena avatar May 18 '22 11:05 Josh-Cena