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

Minor tweaks to website #6264

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

clemyan
Copy link
Member

@clemyan clemyan commented May 2, 2024

What's the problem this PR addresses?

Some minor/nitpick-level problems with the website

Note: This PR has some overlap with #6218. I'll rebase one when the other is merged

How did you fix it?

  • Removed remnants of TypeScript misconfiguration created when first adding the docusaurus workspace via create-docusaurus
  • Reorganized the directory structure of the docusaurus workspace. In particular, moved stuff that is run in build-time (except the docusaurus.config.ts itself) to a config directory. May not seem like much but as more stuff gets added this can keep thing clean and manageable.
  • Used admonitions instead of plain text where appropriate
  • Made the remark plugins apply to the CHANGELOG (they weren't before)
  • Cleaned up and reorganized the dependencies. Of course there are different schools of philosophy regarding what should count as a dependency vs devDependency in a frontend app. Ultimately I decided for Docusaurus, it makes more sense to say everything needed to run yarn build successfully is a dep and devDeps are those that are purely for DX (e.g. types)
  • Removed babel and browserslist config as we are not using them at all

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

I bumped the client bundle ES version based on the original browserslist
config (which was not used). On second thought, let's not introduce a
potentially breaking change in this PR.
tsconfig.json Outdated
Comment on lines 4 to 5
"target": "ES2022",
"lib": ["ES2023"],
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was under the impression that the ES bump isn't because that's what TypeScript recommends for Node 18 and is supported by node.green. Turns out, they are both wrong.

Removing the DOM lib should be safe though? It was added in #4851, most likely because create-docusaurus was run on the root instead of under the docusaurus workspace.

@arcanis arcanis requested a review from merceyz May 6, 2024 08:56
@clemyan
Copy link
Member Author

clemyan commented May 7, 2024

Oh and I forgot to mention, regarding the dependencies, there are a few that I don't know what to do with them

  • @codesandbox/sandpack-react: as far as I can tell this was used for the sherlock playground? I tried to remove it but that would remove a lot of transitive deps and I am weary of introducing churn if and when we rebuild the playground.
  • markdown-it-br and marked-base-url: these are plugins to markdown-it and marked respectively? They are not imported anywhere but I don't know how markdown-it and marked discover/load plugins so I'm not sure whether they are used or not.

For this PR I'm not touching them since there is no harm but I'm generally all for removing unnecessary deps.

Also, in theory, all the default plugins are dependencies of the docusaurus workspace because they are needed to generate the CLI docs. So, in theory we should list all of them as dependencies. This would have the benefit that (together with this PR) that the website can be built with a production-only focused docusaurus workspace but I'm not sure if the maintainence burden is worth that.

Side quesetion: Why is the website built with --no-minify on Netlify?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants