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(utils-build): new utility to build plugins #5612

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Sep 25, 2021

Motivation

Resolve #5564.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Used internally

TODOs

  • Integrate Prettier
  • (After using globs) try not to use internal Babel API
  • 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: feat(v2): Support swizzling TypeScript components #2671

Blocking PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 25, 2021
@netlify
Copy link

netlify bot commented Sep 25, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: f8889f5

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

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

@github-actions
Copy link

github-actions bot commented Sep 25, 2021

⚡️ 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/

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Sep 29, 2021
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.

Looks like it's almost working

  • Not a fan of the package name, we should make it more generic
  • Can we reuse code from babel cli for the watch mode (eventually call the cli?)? Is it really needed to implement our own? Is our implementation similar to Babel cli?
  • Watch mode does not permit to work on theme with hot reload
  • theme / js-theme: unnecessary duplication (main also duplicates, but it's a good opportunity to fix this)
  • Build size bot reports important build size changes, any idea why?

Size Change: -8.3 kB (-1%)

Total Size: 816 kB

Filename Size Change
website/build/assets/js/main.********.js 404 kB -8.29 kB (-2%)
ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 36.7 kB 0 B
website/build/assets/css/styles.********.css 94 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 67 kB 0 B
website/build/blog/index.html 38 kB 0 B
website/build/docs/index.html 44.5 kB -5 B (0%)
website/build/docs/installation/index.html 52.7 kB -5 B (0%)
website/build/index.html 30.8 kB 0 B
website/build/tests/docs/index.html 25.3 kB 0 B
website/build/tests/docs/standalone/index.html 22.9 kB 0 B

packages/docusaurus-theme-classic/package.json Outdated Show resolved Hide resolved
packages/docusaurus-utils-plugin/src/watch.ts Outdated Show resolved Hide resolved
packages/docusaurus-utils-plugin/src/watch.ts Outdated Show resolved Hide resolved
packages/docusaurus-utils-plugin/src/watch.ts Outdated Show resolved Hide resolved
'@babel/plugin-proposal-nullish-coalescing-operator',
'@babel/plugin-proposal-optional-chaining',
],
})?.code ?? ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

when does fallback happen? empty file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Babel doc isn't clear on this, and I've just kept it for testing purposes (it did return null a while ago). I'll keep in mind to remove & retest when we're merging this

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now I'd rather throw if code is undefined, this way we'll find out the undocumented edge cases more easily and decide how to fix them

packages/docusaurus-utils-plugin/src/watch.ts Outdated Show resolved Hide resolved
packages/docusaurus-utils-plugin/package.json Outdated Show resolved Hide resolved
@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Sep 29, 2021

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

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2021

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
Copy link
Collaborator

slorber commented Sep 29, 2021

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

@Josh-Cena
Copy link
Collaborator Author

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 Josh-Cena force-pushed the plugin-util branch 2 times, most recently from c986155 to 9094c89 Compare October 16, 2021 00:39
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena Josh-Cena changed the title feat: new utility to build plugins feat(utils-build): new utility to build plugins Oct 17, 2021
@Josh-Cena Josh-Cena marked this pull request as draft November 3, 2021 10:07
@armano2
Copy link
Contributor

armano2 commented Nov 16, 2021

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

traverse(ast, {
JSXElement(path) {
if (
!path
.get('openingElement')
.get('name')
.isJSXIdentifier({name: 'Translate'})
) {
return;

<_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 :)

@slorber
Copy link
Collaborator

slorber commented Nov 16, 2021

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?

@armano2
Copy link
Contributor

armano2 commented Nov 17, 2021

What are the advantages of using a plugin for you?

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

@slorber
Copy link
Collaborator

slorber commented Nov 18, 2021

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?

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented May 18, 2022

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

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.

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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose unified util to pack TypeScript plugins
4 participants