refactor(docusaurus-init): cleanup #4571

Nytelife26 · 2021-04-04T15:52:01Z

Motivation

(Write your motivation here.)

Have you read the Contributing Guidelines on pull requests?

I have followed all contributing guidelines and signed the CLA accordingly.

Test Plan

The primary changes made are not functionality, but rather performance
improvements and code refactoring. I have verified these changes pass all
available tests and both used the refactored CLI myself and used yarn start
to ensure the site templates build and render correctly. This should require no
additional jest tests.

Related PRs

There is no changed functionality involved with this PR, merely improvements
over existing functionality. I am not currently aware of PRs that involve the
aforementioned functionality but I will link to them if I find them.

netlify · 2021-04-04T15:59:18Z

[V2]

Built without sensitive environment variables with commit ebe0a70

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

github-actions · 2021-04-04T15:59:28Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	86
🟢 Accessibility	96
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	95

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

Foxeye-Rinx · 2021-04-04T20:59:00Z

Can you squash the last commit style(docusaurus-init): apply lint to the second commit refactor(docusaurus-init): switch chalk -> colorette?

Nytelife26 · 2021-04-05T11:01:55Z

@Foxeye-Rinx I could, but I was under the assumption the commits would get squashed upon merge. I'll do it now either way, though.

refactor(docusaurus-init): switch chalk -> colorette refactor(docusaurus-init): cleanup main script chore(docusaurus-init): reformat readme refactor(docusaurus-init/templates): cleanup bootstrap refactor(docusaurus-init/templates): cleanup facebook style(docusaurus-init): apply lint

slorber · 2021-04-05T16:16:15Z

Hi and thanks for your contribution.

I'm sorry but I'm going to close this PR for a few reasons:

This PR does not provide value but refactors the code: there's always a risk with that kind of refactor, so there should be a clearer documented benefit. You mention performance improvements but it's not clear to me how the code your refactor improves performance in any way, unless you provide benchmark. In case the performance gain is not significant or the code more readable, it may not be worth it to do a change.
This PR tries to do too much at once: if we agree we want a refactor, it's unlikely we'll agree on all of them: you'd better split the PR in multiple smaller PRs with a smaller refactor scope and more details about your motivations

If you want to contribute by refactoring, the best way could be to first discuss about the refactors you plan to do, or contact me on discord to know if the refactors you plan to do are welcome

slorber · 2021-04-05T16:16:55Z

packages/docusaurus-init/templates/facebook/src/pages/index.js

@@ -52,11 +53,11 @@ function Feature({imageUrl, title, description}) {
  const imgUrl = useBaseUrl(imageUrl);
  return (
    <div className={clsx('col col--4', styles.feature)}>
-      {imgUrl && (
+      <If condition={imgUrl}>


this is a React anti-pattern and not somthing we'll add

Even if you don't want to make code more readable that way, optional chaining should still be used in other places. features && features.length > 0 is clunky and unnecessary.

We definitively don't want to use the <If> antipattern,

At the time this code was written, optional chaining did not exist, and this code is not updated too frequently. I'm not against using optional chaining now, but keep in mind that features && features.length > 0 is not particularly clunky. In fact, for a non-frontend dev, it might be easier to understand compared to optional chaining. Keep in mind not all Docusaurus users are frontend devs, some are coming from other languages, are not aware of optional chaining, or are from academics background, and they don't care much about using top-notch ES features

slorber · 2021-04-05T16:18:47Z

packages/docusaurus-init/bin/index.js

@@ -7,27 +7,25 @@
 * LICENSE file in the root directory of this source tree.
 */

-const chalk = require('chalk');
+const color = require('colorette');


Why do we want colorette instead of chalk? how can you be sure that this change has no impact for all users and all OS? Why do we want this change only in one package instead of all of them?

Colorette is faster and, well, it's a better package. It doesn't make use of weird prototyping, is solely functional, and if anything provides more support with NO_COLOR (I'm not sure if Chalk does, but they don't mention it).

Colorette is faster and, well, it's a better package.

You'll have to justify your args with benchmarks and external links to back up your claims, and also explain how changing from chalk to colorette is a seamless change that is not risky. If you don't do this, I'll have to do this myself, so you put the work on me. The hard work is not to change an import, it is to assess the risk of changing that import. Everyone can change a package import and then leave when things are on fire.

If something breaks for some reason after this refactor, will you be there to fix the issues reported?
Will we live with using both colorette + chalk in the monorepo for a while or do you plan to migrate everything to colorette soon? Is there a risk that we end up with 2 libs for many years if you don't send another PR? I'd prefer to make the change atomic to the whole codebase at once if we agree that we should use colorette.

slorber · 2021-04-05T16:20:14Z

packages/docusaurus-init/templates/bootstrap/package.json

@@ -19,7 +19,8 @@
    "@mdx-js/react": "^1.5.8",
    "classnames": "^2.2.6",
    "react": "^17.0.1",
-    "react-dom": "^17.0.1"
+    "react-dom": "^17.0.1",


theme bootstrap is unmaintained currently, we'll make it production ready later but it's not worth to update it atm

slorber · 2021-04-05T16:22:17Z

packages/docusaurus-init/src/index.ts

@@ -22,8 +23,8 @@ function hasYarn(): boolean {
  }
 }

-function isValidGitRepoUrl(gitRepoUrl: string): boolean {


I don't think there is a significant perf issue in docusaurus-init. Do we really need to refactor this file in subtle ways (like changing Regexes), particularly when this part of the codebase has not a strong code coverage?

It could still perform better and if it can, why not go for it? Plus, the code does the same thing, there is no need for additional testing as it is synonymous but a cleaner and more performant version. Code quality should be a priority.

I'm not against these changes, but then please add unit tests.

Plus, the code does the same thing

That's what it looks like (unless there is a hidden typo, which is always a possibility with Regexes) but any change without a test ends up being a risky change for me.

Nytelife26 · 2021-04-06T01:29:28Z

This PR does not provide value

Code quality and cleanliness is incredibly valuable.

it's not clear to me how the code your refactor improves performance in any way

Many, many, many ways. Boolean expression optimization, use of opendir for Dirent as opposed to just readdirSync which uses fewer syscalls and allows for less hacky template type detection, et cetera. It's quite clear to see depending on how familiar with Node you are.

or the code more readable

That was a lot of the intention of the cleanup.

This PR tries to do too much at once

If a relatively small single-package cleanup is too much for a single PR, I'm not entirely sure how Facebook manages to work at any efficient pace, but congratulations to you all for pulling that off.

to know if the refactors you plan to do are welcome

That's what PR discussions and code review is for - I can refactor, discuss the changes I made, and then alter it depending on the achieved conclusion prior to merge. Better for everyone.

slorber · 2021-04-06T09:41:15Z

Code quality and cleanliness is incredibly valuable.

Agree

Many, many, many ways. Boolean expression optimization, use of opendir for Dirent as opposed to just readdirSync which uses fewer syscalls and allows for less hacky template type detection, et cetera. It's quite clear to see depending on how familiar with Node you are.

I see some of these are decent micro-optimizations to performance problems we don't have in the first place, in a critical flow of the Docusaurus experience that does not change often and is currently not covered by tests. Is this really necessary to do these changes? If we do, shouldn't we add tests at the same time to ensure the behavior before/after is the same?

If a relatively small single-package cleanup is too much for a single PR, I'm not entirely sure how Facebook manages to work at any efficient pace, but congratulations to you all for pulling that off.

That does not feel very constructive.

That's what PR discussions and code review is for - I can refactor, discuss the changes I made, and then alter it depending on the achieved conclusion prior to merge. Better for everyone.

Then let's do that and move on: you have my review, feel free to handle it or not.

Just to help you understand better my perspective:

I have limited time and should focus on more important things
You add work to my work queue that I was not willing to do in the first place
Reviewing and merging this PR is time consuming, requires discussions, assessing the risk/reward of each refactor to ensure the next release is not broken

Refactors are welcome, but please:

make your PR less time-consuming to review and easier to merge
don't do too many things at once, do the hard work of risk assessment and document correctly the perf benefits, and the potential risks of the changes

Examples PRs:

Migrate every package to colorette, document correctly the benefits, show benchmarks, explain why this change is not risky, provide official external links to backup your claims (because otherwise, I'll have to do that job myself)
Add tests to an existing package, as you plan to refactor it later
Replace sync code with async code, make sure tests still pass

Those 3 PRs would be much easier to merge for me

chore: switch chalk -> colorette

df9f4c3

Nytelife26 requested review from lex111 and slorber as code owners April 4, 2021 15:52

facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 4, 2021

Nytelife26 force-pushed the refactor/cleanup-docusaurus-init branch from bfd94bc to ebe0a70 Compare April 5, 2021 11:06

slorber closed this Apr 5, 2021

slorber reviewed Apr 5, 2021

View reviewed changes

Nytelife26 mentioned this pull request Apr 6, 2021

refactor(*): switch chalk -> colorette #4579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(docusaurus-init): cleanup #4571

refactor(docusaurus-init): cleanup #4571

Nytelife26 commented Apr 4, 2021

netlify bot commented Apr 4, 2021 •

edited

Loading

github-actions bot commented Apr 4, 2021 •

edited

Loading

Foxeye-Rinx commented Apr 4, 2021

Nytelife26 commented Apr 5, 2021

slorber commented Apr 5, 2021

slorber Apr 5, 2021

Nytelife26 Apr 6, 2021

slorber Apr 6, 2021

slorber Apr 5, 2021

Nytelife26 Apr 6, 2021

slorber Apr 6, 2021

slorber Apr 5, 2021

slorber Apr 5, 2021

Nytelife26 Apr 6, 2021

slorber Apr 6, 2021

Nytelife26 commented Apr 6, 2021

slorber commented Apr 6, 2021

refactor(docusaurus-init): cleanup #4571

refactor(docusaurus-init): cleanup #4571

Conversation

Nytelife26 commented Apr 4, 2021

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

netlify bot commented Apr 4, 2021 • edited Loading

github-actions bot commented Apr 4, 2021 • edited Loading

Foxeye-Rinx commented Apr 4, 2021

Nytelife26 commented Apr 5, 2021

slorber commented Apr 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nytelife26 commented Apr 6, 2021

slorber commented Apr 6, 2021

netlify bot commented Apr 4, 2021 •

edited

Loading

github-actions bot commented Apr 4, 2021 •

edited

Loading