migrate frontend to docusaurus #9014
Conversation
I would quite like to re-add this at some point but its not really the top priority thing right now
chris48s
added
frontend
|
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again. Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. What is network access?This module accesses the network. Packages should remove all network access that isn't functionally unnecessary. Consumers should audit network access to ensure legitimate use. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
| camp.route(/\.png$/, (queryParams, match, end, ask) => { | ||
| camp.route(/^\/((?!img\/)).*\.png$/, (queryParams, match, end, ask) => { |
There was a problem hiding this comment.
This is to allow /img/logo.png to work without trying to redirect to squint.
This does bring up a slightly wider question though: The way I've done this, I have put the frontend in the root so the compiled website structure (assets, badges, community, search, etc) lives in the same namespace as the badge URLs. This is pretty much what we did with gatsby too but because every badge has its own URL for the builder, we could consider putting the frontend in /docs or whatever and make / a redirect to /docs. I think we should really decide this now because once we launch this version of the site there will be a bunch of links out there in the wild which will break if we change our minds. Thoughts?
There was a problem hiding this comment.
do you have a feel for which may give us more flexibility over the long term? Also, I gather that /docs was just an example but given that we have an existing doc directory I think we'd want to use something with a more differentiated name for the generated site output
There was a problem hiding this comment.
So if we followed that suggestion, the site frontend would live at https://shields.io/docs and https://shields.io/ would redirect to https://shields.io/docs
The code (or output) would not need to live in https://github.com/badges/shields/tree/master/docs in the repo though.
Basically the advantage of that is it would get all the frontend routes out of the same namespace we use for the badge routes but at the moment we don't have any conflicts to worry about.
There was a problem hiding this comment.
I hate trying to mentally parse regexes, but last, semi-pathological question on this.. would this potentially match any cases that should get redirected to squint?
e.g. a repo named img, perhaps like https://img.shields.io/github/v/tag/pathological-username/img/some-branch.png
There was a problem hiding this comment.
If that's not the case and just some bad mental regex parsing and hypothesizing then great, if we think that's an extremely fringe edge case hypothetical that's not worth the gymnastics to try to work around then I'm also good with that; just wanted to think through possibilities.
There was a problem hiding this comment.
Yeah good question.
img would have to be the first "clause", so it would clash if we tried to add a service called img, but it is fine if there is a package or repo called img, as in your example.
The first part of the regex is:
^- beginning of the string\/- escaped slash character((?!img\/))- anything other thanimg
frontend/docusaurus.config.cjs
Outdated
| /** @type {import('@docusaurus/types').Config} */ | ||
| const config = { | ||
| title: "Shields.io", | ||
| tagline: "Concise, consistent, and legible badges", |
There was a problem hiding this comment.
I think we should stop "promoting" raster format
There was a problem hiding this comment.
I don't have a strong position on this but will spend some time contemplating. My initial inclination is something along the lines of...
yeah, we probably don't need to go out of our way to push them, but, we do still provide them because they are helpful in a few contexts and it's helpful for new users to know that we do offer them
though i'm trying to challenge myself to gauge whether i'm just being biased given my prior work on squint
There was a problem hiding this comment.
The reason I said that specifically here is we currently sometimes use the tag line "Concise, consistent, and legible badges in SVG and raster format". I've dropped "in SVG and raster format" from that. I don't think it needs to be in the first line someone reads about the project. We don't necessarily need to completely stop documenting that png exists as an option though.
I can't remember if raster made its ways into this anywhere, but let me take an action to make sure it is in there somewhere.
| "check-types:package": "tsd badge-maker", | ||
| "check-types:frontend": "tsc --noEmit --project .", |
There was a problem hiding this comment.
Given how little of our own frontend code we are going to be left with, I think the value of typescript or unit tests for the frontend is limited. I think I'm going to write a few e2e tests and leave it at that.
There was a problem hiding this comment.
We will retain the typescript checks for the package types though
There was a problem hiding this comment.
The other check we can perform on the frontend is ensuring npm run build completes without errors. I haven't added n explicit CI build for this. It is covered implicitly because it happens in both the E2E tests and Dockerfile builds.
There was a problem hiding this comment.
i agree with the sentiment, but want to get a feel for the scope of TS left before fully forming my opinion (i'm starting my review by perusing your remaining comments like this one, and haven't really dug in to the meat of the diff yet)
There was a problem hiding this comment.
This PR drops almost all typescript code and tooling. The exceptions to that are:
- badge-maker types:
./badge-maker/index.d.ts./badge-maker/index.test-d.ts
- We are still using
@typescript-eslint/parseras out parser for ESLint. This is because the standard ESLint parser doesn't implement support for any node js feature that is not stable yet. This includes some features we use (one of them is top-levelawait, but there are others). As well as typescript stuff,@typescript-eslint/parseralso implements compatibility with a broader subset of node features. So that is why we're using a typescript parser, despite mostly not using typescript. Upgrading from ESLint 7 to ESLint 8 might allow us to drop@typescript-eslint/parserand switch to using the standard ESLint parser but I have not tried it. It is another issue for anoter day.
That's it.
This isn't necessarily some crusade against type safety. It just happens that the only place in the codebase where we were using typescript was the frontend and we're moving to an approach where we are mostly relying on packages and there is very little custom frontend code.
There was a problem hiding this comment.
Understood, and there were never any implications of crusades. FWIW I'm a big fan of TypeScript, but there's a point of metaphorical "critical mass" that has to be reached in order for me to feel the benefits justify the overhead (extra tooling/dependencies, build process, etc.). I think we were/are arguably past that point previously/currently (at least one could reasonably make the case we were), so I was just noting that I intended on making that same personal assessment with these proposed changes.
I don't have any predispositions one way or the other, though based on the way you've described the changes I thoroughly anticipate I'd reach the same conclusion as you in that removal of TS is the right approach, and that the overhead of keeping TS around would outweigh any benefits in the new context.
There was a problem hiding this comment.
Docusaurus still doesn't support the config file being an ESModule so there's a few files of CommonJS here and /frontend still has its own special package.json to allow it to be a CJS module
| @@ -65,7 +65,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) { | |||
| */ | |||
| if (match[0] === '/endpoint' && Object.keys(queryParams).length === 0) { | |||
| ask.res.statusCode = 301 | |||
| ask.res.setHeader('Location', '/endpoint/') | |||
| ask.res.setHeader('Location', '/badges/endpoint-badge') | |||
There was a problem hiding this comment.
I haven't bothered with redirects for the category pages. We could do, but there isn't a one-to-one replacement for them
There was a problem hiding this comment.
may be misunderstanding but this leads me to two questions:
- does this mean we can no longer directly link to a page that enumerates badges under a specific category?
- if that's wrong (and we can still do so), does this mean we'd break any existing links? (note that's not the end of th world imo, just want to understand implications)
There was a problem hiding this comment.
Yep. Every badge gets its own page in the docs now, but there are no pages for the categories. The categories are groupings in the menu but not pages in their own right.
There was a problem hiding this comment.
that's a minor setback imo. i've found it helpful to be able to give folks a link to the category pages in the past. to be clear i don't think that's a blocker, just unfortunate
There was a problem hiding this comment.
I made some contributions upstream to make it easier to override individual React Components in the theme without having to override their parent components to do it, but there are still a couple of react components from docusaurus-theme-openapi that we need to override.
- We need to override the
Responsecomponent so that if the API endpoint returns an image (rather than XML or JSON) we can render it. - We need to override the (badly named)
Curlcomponent so that we can define custom code examples with placeholders and then populate the placeholders at render time.
(this process of overriding a single component is called "swizzling")
There are 2 competing objectives here:
- I want to keep the customised versions as close to the upstream versions as possible - this will make it easier to compare our customised versions of the components against the upstream versions when we upgrade the theme package.
- I want the code in frontend to conform to our normal coding standards (eslint, prettier, etc) both for preventing errors and stylistic considerations
For now, the direction I've gone is I've just matched our "house style" (except for the prop checks, which I disabled) at the expense of some divergence from the upstream but I might review this. If we find we are often trying to diff these against the upstream components I might change the rules for frontend/src/theme/* to make this easier.
| "prestart": "run-s --silent depcheck defs features", | ||
| "start": "concurrently --names server,frontend \"npm run start:server\" \"cd frontend && cross-env GATSBY_BASE_URL=http://localhost:8080 gatsby develop --port 3000\"", | ||
| "prestart": "run-s --silent depcheck defs", | ||
| "start": "concurrently --names server,frontend \"npm run start:server\" \"npm run docusaurus:start\"", |
There was a problem hiding this comment.
When you run the built version of the docs (i.e: npm run build and serve it from scoutcamp), the website performs really well because it is just serving static HTML files.
The dev server (with hot reload when you change the files and all that jazz) is a bit of a resource hog, especially the first time you start it. It does seem to get a bit better once docusaurus has cached some stuff, but your first experience of running npm start is likely to be a bit sluggish. I think the fact we have over 600 pages doesn't help but in general this is an acknowledged issue: facebook/docusaurus#4765 It is not great dev experience, but I can live with it. Its not an issue that will affect production. This was kinda true of gatsby too, but docusaurus is way worse. If it turns into a big pain point, we could consider having 2 variants of the dev server:
- A "I want to work on the website" command (that runs
docusaurus startso you get hot reloads for the frontend) and - A "I want to work on the badge server" command (that runs
npm run buildand serves it from scoutcamp)
For now I am going to ignore this issue.
There was a problem hiding this comment.
The other possibility is that it might be possible to tweak webpack or use a different bundler. I don't think its a blocker though.
There was a problem hiding this comment.
Just thinking this through a bit more..
If we think the performance of the webpack devserver is terrible, I think changing the default behaviour of npm start in dev to just compile the frontend and serve it with no watch on the frontend (but keep nodemon doing hot reloads for the backend) does not actually lose us very much. I've deleted almost all of the frontend code so there is less to watch :) I think it would be useful to get another opinion on the dev experience here.
There was a problem hiding this comment.
i don't think i'll have time today but i'll pull your branch down locally and give it a test drive soon
|
TODOs that need to happen in this PR:
TODOs that I will punt to later:
|
There was a problem hiding this comment.
I tend to not like mixing in proper case nor camel case into file names in order to be friendlier on case-sensitive platforms and file systems. Is this something we have autonomy to change, or does Docusaurus or something else need this exact name?
There was a problem hiding this comment.
Nope. That is change-able. Done in b2ff593
| ] | ||
| const languageTheme = { | ||
| plain: { | ||
| color: 'var(--ifm-code-color)', |
There was a problem hiding this comment.
I'm guessing we've pulled/copied this from somewhere, and no concerns with that, but did want to note that the specifics of the args being passed to these function calls (I presume) are a complete black box to me 🤷
There was a problem hiding this comment.
Yep. So this is one of the components from docusaurus-theme-openapi I have customised.
Its basically a copy of
https://github.com/cloud-annotations/docusaurus-openapi/blob/0beb11b248e34e2b88f171406fc4fb04a390f065/packages/docusaurus-theme-openapi/src/theme/ApiDemoPanel/Curl/index.tsx with some customisations.
I think this is mostly covered in
#9014 (comment)
| This is a special case for production. | ||
|
|
||
| We want to be able to build the front end with no value set for | ||
| `BASE_URL` so that staging, prod and self hosting users | ||
| can all use the same docker image. | ||
|
|
||
| When deployed to staging, we want the frontend on | ||
| https://staging.shields.io/ to generate badges with the base | ||
| https://staging.shields.io/ | ||
| (and we want similar behaviour for users hosting their own instance) | ||
|
|
||
| When we promote to production we want https://shields.io/ and | ||
| https://www.shields.io/ to both generate badges with the base | ||
| https://img.shields.io/ | ||
|
|
||
| For local dev, we can deal with setting the api and front-end | ||
| being on different ports using the BASE_URL env var |
There was a problem hiding this comment.
I know we already employ this so consider this a tangential future topic, but with our current hosting setup, I wonder if we've got access to do the www -> non-www redirect at some proxy/traffic manager level (e.g. if Fly has something like nginx or haproxy in there in a way that we could configure/use to add the redirect
There was a problem hiding this comment.
I think the current answer to this is "no" https://community.fly.io/t/how-to-redirect-from-non-www-to-www/5795
| } | ||
|
|
||
| function Curl({ postman, codeSamples }) { | ||
| // TODO: match theme for vscode. |
There was a problem hiding this comment.
Is this a real todo for us or a copy pasta from somewhere else?
There was a problem hiding this comment.
Nope it is from the upstream
https://github.com/cloud-annotations/docusaurus-openapi/blob/0beb11b248e34e2b88f171406fc4fb04a390f065/packages/docusaurus-theme-openapi/src/theme/ApiDemoPanel/Curl/index.tsx#L126
In #9014 (comment)
I've touched on the topic of why I've tried to keep the changes I've made minimal in the section starting "There are 2 competing objectives here..."
Essentially: if a new version of the upstream package is released, I want it to be reasonably easy to diff our customised version against the package version to see if there are any changes we need to make (this is something I'd like to make some CI tooling for, but.. later).
|
Have finally done a full pass through this and think it's in good shape. I don't have any blocking concerns nor issues, just a few thoughts and questions left inline. There's only two things I'd like to give a spin/glance before moving ahead. I anticipate having time to do both this week, so I'd suggest we leave the remainder of the week for that, and then once that's done or if Sunday comes, I'd be good with resolving the conflicts and moving ahead (whenever you have the bandwidth) Two things I'd still like to do:
Finally, thanks again for taking this on. It's been a long standing desired and need, and was obviously a major, time consuming haul. |
|
🚀 Updated review app: https://pr-9014-badges-shields.fly.dev |
|
This review is getting pretty disjointed, so just to try and make it super-clear. Here's a list of links to the comments I've left in this pass: |
To help out with this, I've merged all this week's package bumps and resolved all the conflicts. There is a review app up at https://pr-9014-badges-shields.fly.dev/ If it has deleted itself, you can re-deploy it
I think I'd ideally like to get this merged and deployed next week. If you're able to find a few mins to have a poke about in the review app on Sunday that would be absolutely ideal 🙏 |
|
🚀 Updated review app: https://pr-9014-badges-shields.fly.dev |
|
I'm on pretty pedestrian hardware and although my first I don't view impact on the inner dev loop as a deal breaker, whatsoever |
|
Not sure if this is a quirk, bug, or pebcak, but any ideas what's happening with these seemingly duplicative Azure DevOps badge listings in the sidebar?
As compared to what we have today:
I feel like I may have seen this elsewhere too but didn't register it at the time while browsing |
|
🚀 Updated review app: https://pr-9014-badges-shields.fly.dev |
Yep, so this is something that we've touched on before Definitely in #7982 (comment) but worth a recap. In OpenAPI, query string params can be optional but there is no such thing as an optional path (positional) parameter, so you can't do {
"name": "branch",
"in": "path",
"required": false, // nope
"description": "Branch",
"schema": { "type": "string" }
}This means we need to generate a separate page for every with/without optional param variant. shields/core/base-service/openapi.js Lines 91 to 119 in 2651e5f So the difference between
is one has a Also, separately from that, we've also got a bunch of places where we already have duplicated titles in the existing examples e.g:
which we also replicate on the new site. I am very aware that having multiple menu items with the same title is unhelpful for discovery and navigation now that the only thing the user can see is the title, but one of the ways I'm trying to avoid boiling the whole ocean at once is by separating platform and content a bit. As an opening gambit to get the "platform" bit of the migration done, I'm comfortable if every bit of content is not perfect to start with. Once we move to defining the open api definitions on the service classes directly, there's a lot of content improvements I'd like to make (once we've got the right structure in place, it will be possible to do this in lots of smaller simpler "improve docs for service X" PRs rather than mixing it in with the platform migration) and sorting out duplicate titles is going to be one of those jobs. Maybe once we get to a point where each one has a unique title, we can add a check somewhere (maybe the service loader) to ensure all the titles are unique. |
Believe I follow the prior portion of the response explaining the presence of the duplicative titles, and at least AIUI, the same underlying reasoning also explains the absence of some titles (and corresponding different badges). I think the platform -> content sequencing is a reasonable approach, although along this aspect 👇
I'm certainly having trouble maintaining a mental model of all these aspects, though admittedly the size of changes and the delta between the windows I'm actively thinking about it are both certainly aggravating factors. Do we have anything like a high level tracking issue with a succinct enumeration of some of these content-related items we're aware of and deferring to follow up work? (and apologies if we do, my lack of awareness of such an issue speaks back to that failing mental model 🧠). If not, do you think it would be reasonable to pull something like that together? I know you've probably got such a list in your head and I'm sure the items that would constitute such a list are covered within various Issue and PR discussions, but I feel like having things captured in one place would make them more easily consumable and trackable |
|
Yep so the top post of #8966 starting from
was my last attempt to do that and is still broadly accurate. Obviously this PR is the first thing in that list. There are probably some other things that do exist in my head or as bits where I've said "This is a bit crap - we should make it better" somewhere in a diff comment on this PR or on #8966 at the moment that need extracting into issues. One of the next things I want to do after we merge this is go through https://github.com/badges/shields/issues?q=is%3Aopen+is%3Aissue+label%3Afrontend work out what (if any of it) is still relevant, but probably close all/most of them. Then write up a bunch of new frontend issues for the next steps on this with some proper details. If you want though I can try to pull all that together into a rough bullet list here tomorrow evening before we merge this. |
|
Right. Here's a quick summary of next steps I want to work on after merging this. Probably not in this precise order:
I can write those up into proper issues with more info |
|
today is the day |
* delete loads of really important stuff that we definitely need * v basic MVP smoosh docusaurus PoC into repo * TODO * delete more really important stuff * TODO * tidyup: use run-s * don't redirect images used in frontend to raster proxy * fix routing * preserve the /endpoint link * delete the blog (for now) I would quite like to re-add this at some point but its not really the top priority thing right now * content edits * appease the lint gods * update danger rules * remove placeholder * cypress tests * dockerhub --> ghcr * Revert "dockerhub --> ghcr" This reverts commit ef74cbb. * downgrade lockfile format * implement defs/BASE_URL * fix e2e build * actually fix cypress tests * always run cypress tests on build * this never worked * add command for docusaurus:clear * delete more code we don't need any more * update ESLint/prettier config * delete unsused exports * documentation updates * delete a fairly large chunk of our dependency tree * allow base_url as build arg to Dockerfile * fixup dockerfile * work out base url at runtime if not set doing this at image build time is not the right approach * remove gatsby monorepo from closebot * rename HomepageFeatures to homepage-features
Refs #7982
This PR is now ready for review.
As with #8966 the commit history for this isn't very useful. Sorry. If it is just too much of a mess, let me know and I will try to do a massive rebase/squash and clean it up a bit.
I've set up a review app at https://pr-9014-badges-shields.fly.dev/ which won't delete itself for the moment because we haven't merged #9069 yet.
I've been though all the comments. Anything that is resolved or hidden is now irrelevant. Anything still unresolved is still relevant. Note that I've spent so much time chatting away to myself in this PR that Github has started to hide bits of it.
Once we get this merged/deployed, I'll start writing up the next steps from this PR and #8966 into issues which we can hopefully tackle a bit more incrementally.
I'll need to remove test-frontend from the required checks in settings when we merge this.
I've attempted to give us some basically sensible content as an opening gambit, but we can always fiddle with content later. I'm mainly trying to get the tech bit right in this PR.