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

migrate frontend to docusaurus #9014

Merged
merged 38 commits into from
Jun 17, 2023
Merged

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Mar 20, 2023

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.

Screenshot at 2023-04-15 19-52-29

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.

@chris48s chris48s added frontend The Docusaurus app serving the docs site blocker PRs and epics which block other work labels Mar 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2023

Warnings
⚠️

Found 'assert' statement added in core/base-service/service-definitions.js.
Please ensure tests are written using Chai expect syntax

⚠️

📚 Remember to ensure any changes to config.private in services/github/auth/acceptor.js are reflected in the server secrets documentation

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against 5d6ff7e

@socket-security
Copy link

socket-security bot commented Mar 20, 2023

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Issue Package Version Note Source
Install scripts postman-code-generators 1.7.2
  • Install script: postinstall
  • Source: node npm/deepinstall.js
package-lock.json via docusaurus-preset-openapi@0.6.4
Network access faye-websocket 0.11.4 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access http2-client 1.3.5 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access superagent 7.1.6 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access @reduxjs/toolkit 1.9.5 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access clean-css 5.3.2 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access docusaurus-theme-openapi 0.6.4 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access monaco-editor 0.31.1 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access node-fetch-h2 2.3.0 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access oas-resolver-browser 2.3.3
  • Module: globalThis["fetch"]
  • Location: index.js
package-lock.json via docusaurus-preset-openapi@0.6.4
Network access sanitize-html 1.20.1 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access webpack-dev-server 4.15.1 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access @algolia/requester-node-http 4.17.2 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access http-proxy 1.18.1 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access multicast-dns 7.2.5 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access default-gateway 6.0.3 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access postman-collection 3.6.6 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access postman-collection 4.0.0 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access postman-collection 4.1.7 package-lock.json via docusaurus-preset-openapi@0.6.4
Network access serve-index 1.9.1
  • Module: http-errors
  • Location: index.js
package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access websocket-driver 0.7.4 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access @docusaurus/core 2.4.1 package-lock.json, package.json via @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access sockjs 0.3.24 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access webpack-bundle-analyzer 4.9.0 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access bonjour-service 1.1.1 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4
Network access http-proxy-middleware 2.0.6 package-lock.json via @docusaurus/core@2.4.1, @easyops-cn/docusaurus-search-local@0.33.6, docusaurus-preset-openapi@0.6.4

Next steps

What 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 dependency

Take 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 package

If 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 risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore postman-code-generators@1.7.2
  • @SocketSecurity ignore faye-websocket@0.11.4
  • @SocketSecurity ignore http2-client@1.3.5
  • @SocketSecurity ignore superagent@7.1.6
  • @SocketSecurity ignore @reduxjs/toolkit@1.9.5
  • @SocketSecurity ignore clean-css@5.3.2
  • @SocketSecurity ignore docusaurus-theme-openapi@0.6.4
  • @SocketSecurity ignore monaco-editor@0.31.1
  • @SocketSecurity ignore node-fetch-h2@2.3.0
  • @SocketSecurity ignore oas-resolver-browser@2.3.3
  • @SocketSecurity ignore sanitize-html@1.20.1
  • @SocketSecurity ignore webpack-dev-server@4.15.1
  • @SocketSecurity ignore @algolia/requester-node-http@4.17.2
  • @SocketSecurity ignore http-proxy@1.18.1
  • @SocketSecurity ignore multicast-dns@7.2.5
  • @SocketSecurity ignore default-gateway@6.0.3
  • @SocketSecurity ignore postman-collection@3.6.6
  • @SocketSecurity ignore postman-collection@4.0.0
  • @SocketSecurity ignore postman-collection@4.1.7
  • @SocketSecurity ignore serve-index@1.9.1
  • @SocketSecurity ignore websocket-driver@0.7.4
  • @SocketSecurity ignore @docusaurus/core@2.4.1
  • @SocketSecurity ignore sockjs@0.3.24
  • @SocketSecurity ignore webpack-bundle-analyzer@4.9.0
  • @SocketSecurity ignore bonjour-service@1.1.1
  • @SocketSecurity ignore http-proxy-middleware@2.0.6

package.json Outdated Show resolved Hide resolved
camp.route(/\.png$/, (queryParams, match, end, ask) => {
camp.route(/^\/((?!img\/)).*\.png$/, (queryParams, match, end, ask) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 than img

/** @type {import('@docusaurus/types').Config} */
const config = {
title: "Shields.io",
tagline: "Concise, consistent, and legible badges",
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 think we should stop "promoting" raster format

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
"check-types:package": "tsd badge-maker",
"check-types:frontend": "tsc --noEmit --project .",
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will retain the typescript checks for the package types though

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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/parser as 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-level await, but there are others). As well as typescript stuff, @typescript-eslint/parser also 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/parser and 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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')
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 haven't bothered with redirects for the category pages. We could do, but there isn't a one-to-one replacement for them

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.

  1. We need to override the Response component so that if the API endpoint returns an image (rather than XML or JSON) we can render it.
  2. We need to override the (badly named) Curl component 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:

  1. 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.
  2. 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\"",
Copy link
Member Author

Choose a reason for hiding this comment

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

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 start so you get hot reloads for the frontend) and
  • A "I want to work on the badge server" command (that runs npm run build and serves it from scoutcamp)

For now I am going to ignore this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

i don't think i'll have time today but i'll pull your branch down locally and give it a test drive soon

@chris48s
Copy link
Member Author

chris48s commented Mar 20, 2023

These lists probably are not complete. Also needs a general sweep. Grep for frontend, gatsby, etc

TODOs that need to happen in this PR:

TODOs that I will punt to later:

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. That is change-able. Done in b2ff593

]
const languageTheme = {
plain: {
color: 'var(--ifm-code-color)',
Copy link
Member

Choose a reason for hiding this comment

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

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 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Comment on lines +107 to +123
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
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a real todo for us or a copy pasta from somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

@calebcartwright
Copy link
Member

calebcartwright commented Jun 5, 2023

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:

  • Seeing the frontend live with these changes (the review app has been down for me ever since I first looked at this PR. that being said, I should be able to rebuild the app or fix that myself)
  • Pulling things down locally and launching the server to get a feel for the inner dev loop experience discussed above.

Finally, thanks again for taking this on. It's been a long standing desired and need, and was obviously a major, time consuming haul.

@socket-security
Copy link

New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives1 Size Publisher
docusaurus-preset-openapi 🆕 0.6.4 None +578 230 MB bourdakos1
@easyops-cn/docusaurus-search-local 🆕 0.33.6 filesystem, environment +451 236 MB easyops-eve
@docusaurus/core 🆕 2.4.1 eval, network, filesystem, shell, environment +404 42.1 MB slorber
clsx 🆕 1.2.1 None +0 5.67 kB lukeed
@mdx-js/react 🆕 1.6.22 None +1 30.6 kB johno
prism-react-renderer 🆕 1.3.5 None +1 386 kB gksander
graphql ⬆️ 15.7.2...15.8.0 None +0/-0 2.12 MB i1g
@renovatebot/ruby-semver ⬆️ 2.1.11...2.1.12 None +1/-1 114 kB renovate-bot
@typescript-eslint/parser ⬆️ 5.58.0...5.59.9 None +27/-28 3.77 MB jameshenry

🚮 Removed packages: @babel/polyfill@7.12.1, @babel/register@7.22.5, @fontsource/lato@5.0.2, @fontsource/lekton@5.0.2, @mapbox/react-click-to-select@2.2.1, @types/chai@4.3.5, @types/lodash.debounce@4.0.7, @types/lodash.groupby@4.6.7, @types/mocha@10.0.1, @types/node@16.7.10, @types/react-helmet@6.1.6, @types/react-modal@3.16.0, @types/react-select@4.0.17, @types/styled-components@5.1.26, @typescript-eslint/eslint-plugin@5.59.2, babel-plugin-inline-react-svg@2.0.2, babel-preset-gatsby@2.23.0, clipboard-copy@4.0.1, fetch-ponyfill@7.1.0, gatsby@4.23.1, gatsby-plugin-catch-links@4.25.0, gatsby-plugin-page-creator@4.25.0, gatsby-plugin-react-helmet@5.25.0, gatsby-plugin-remove-trailing-slashes@4.9.0, gatsby-plugin-styled-components@5.24.0, gatsby-plugin-typescript@4.25.0, humanize-string@2.1.0, js-yaml-loader@1.2.2, react-helmet@6.1.0, react-modal@3.16.1, react-pose@4.0.10, react-select@4.3.1, styled-components@5.3.11, ts-mocha@10.0.0

Footnotes

  1. https://docs.socket.dev

@github-actions
Copy link
Contributor

🚀 Updated review app: https://pr-9014-badges-shields.fly.dev

@chris48s
Copy link
Member Author

@chris48s
Copy link
Member Author

I'd like to give a spin/glance before moving ahead.

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

Screenshot at 2023-06-10 17-32-12

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 🙏

@github-actions
Copy link
Contributor

🚀 Updated review app: https://pr-9014-badges-shields.fly.dev

@calebcartwright
Copy link
Member

I'm on pretty pedestrian hardware and although my first npm start wasn't what I'd describe as snappy, I also don't think it was all that noticeably worse than my experience with gatsby.

I don't view impact on the inner dev loop as a deal breaker, whatsoever

@calebcartwright
Copy link
Member

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?

image

As compared to what we have today:

image

I feel like I may have seen this elsewhere too but didn't register it at the time while browsing

@github-actions
Copy link
Contributor

🚀 Updated review app: https://pr-9014-badges-shields.fly.dev

@chris48s
Copy link
Member Author

any ideas what's happening with these seemingly duplicative Azure DevOps badge listings

Yep, so this is something that we've touched on before

Definitely in #7982 (comment)
and probably somewhere in the review for #8966

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.
If we haven't already got 2 separate examples for each with/without optional route param variant, I'm creating the variants using

function getVariants(pattern) {
/*
given a URL pattern (which may include '/one/or/:more?/:optional/:parameters*')
return an array of all possible permutations:
[
'/one/or/:more/:optional/:parameters',
'/one/or/:optional/:parameters',
'/one/or/:more/:optional',
'/one/or/:optional',
]
*/
const patterns = [pattern.split('/')]
while (patterns.flat().find(p => p.endsWith('?') || p.endsWith('*'))) {
for (let i = 0; i < patterns.length; i++) {
const pattern = patterns[i]
for (let j = 0; j < pattern.length; j++) {
const path = pattern[j]
if (path.endsWith('?') || path.endsWith('*')) {
pattern[j] = path.slice(0, -1)
patterns.push(patterns[i].filter(p => p !== pattern[j]))
}
}
}
}
for (let i = 0; i < patterns.length; i++) {
patterns[i] = patterns[i].join('/')
}
return patterns
}
but I'm just doubling up the existing title for now. It is not perfect, but it is workable to get this part of the migration done.

So the difference between

is one has a branch param, and one doesn't.

Also, separately from that, we've also got a bunch of places where we already have duplicated titles in the existing examples e.g:

Screenshot at 2023-06-12 10-52-45

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.

@calebcartwright
Copy link
Member

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.

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 👇

Yep, so this is something that we've touched on before

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

@chris48s
Copy link
Member Author

Yep so the top post of #8966 starting from

Next steps from here (may not be exhaustive) are something like:
[bullets points]

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.

@chris48s chris48s mentioned this pull request Jun 14, 2023
8 tasks
@chris48s
Copy link
Member Author

Right. Here's a quick summary of next steps I want to work on after merging this. Probably not in this precise order:

  • Triage https://github.com/badges/shields/issues?q=is%3Aopen+is%3Aissue+label%3Afrontend and close most of them
  • Sort out spacing issues: There are a number of service classes with minor spacing issues in the help/documentation HTML that need fixing
  • (auto-)convert all the existing examples arrays on service classes to openApi objects (this will be a huge job touching basically every .service.js file in the codebase, but I'm hoping to automate it)
  • Delete all examples properties + remove any validation for it
  • Make openApi property required/validated
  • Delete all the code for bludgeoning legacy examples objects to OpenAPI objects
  • Correctly document required query params
  • Fix duplicated titles
  • Remove unnecessary duplicates for query param variants
  • Start adding param descriptions
  • Update contributing docs for adding new services to refelect new frontend + documenting with openApi property instead of examples
  • Look for ways we can reduce duplication between the service definition/schema and OpenAPI spec
  • Implement some automated checks (be that Joi schema, CI checks - TBC) that help to prevent common issues
  • Automatically flag any URL conflicts between badge server and frontend
  • CI automation to help us diff/update swizzled components when updating docusaurus-theme-openapi
  • ESLint 8
  • Retry Node 16 --> 18 migration. Does getting rid of Gatsby unblock this, or are we now blocked by ScoutCamp?
  • generating lockfile with npm 9 breaks node 14 package build #9075 (NPM 9)

I can write those up into proper issues with more info

@chris48s chris48s mentioned this pull request Jun 15, 2023
@chris48s
Copy link
Member Author

today is the day

@chris48s chris48s merged commit 50ea706 into badges:master Jun 17, 2023
calebcartwright pushed a commit that referenced this pull request Jun 17, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker PRs and epics which block other work frontend The Docusaurus app serving the docs site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants