Third attempt at migrating to rehype by iAdramelk · Pull Request #253 · gravitational/docs

iAdramelk · 2023-03-15T13:29:25Z

Because of the revert, there are a lot of files in the PR. To make the review faster, I split every new fix into separate commits and lined them below.

Changes:

Live Refresh is broken in dev environments. #247 is partially fixed – no need to reload the server anymore, but you still need to reload the pages. Making real hot reload work with getServerSideProps may be impossible or pretty hard. Commit: 9e8b835
Videos not autoplaying #248 - fixes bugs with missing attributes and adds tests for the plugin. Commit: 8124d2b
Unset forced ratio/resolution for Docs Images docs-website#71 - fixes image aspect ratio by updating to Next@13. Next@13 reworked the way images are inserted into the page by removing span wrappers and absolute positioning. It fixed a lot of problems with image proportions. To make it even more stable, I replaced probe-image-size library with the image-size library used by Next.js itself. 9264aac
Support links to the current docs version at /ver/<version> #243 - Make current version URLs with /ver/x.xwork via redirects. Commit: 64f39d8
Fix a minor bug with Var component hydration problem. Commit: 22ab61e
Updated webpack to fix vulnerability warning. Commit: 3bb98fc
Return older-versions page. Commit: 6d6e6d5

#246 is not a part of this PR because it is not related to rehype migration. I will work on it separately.

vercel · 2023-03-15T13:29:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
docs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 16, 2023 at 7:09PM (UTC)

avatus

left some comments/questions. So far, works great locally and on the deployed link.
One thing I couldn't comment on in the code was the removal of older-versions. That page was a "common" page that didn't rely on docs from the content folder. Its a way to prevent us from having to backport information that isn't version specific. I'd like to see it added back in (and there will be more pages like this soon, such as Upcoming Features, so we'd like to be able to parse any other pages that exist besides [[...slug]], although I believe that is already the case since search-results still exists.

avatus · 2023-03-15T17:27:06Z

components/VideoBar/VideoBar.module.css

+.next {
+  object-fit: contain;
+}
+


this name seems a bit ambiguous. perhaps we can call the class object-contain or something, since that's all its really doing. It makes it easier to know what it's doing and makes more sense in the VideoBar component as wel

avatus · 2023-03-15T17:41:23Z

pages/[[...slug]].tsx

+
+// Here we generate a list of pages to render with getStaticProps,
+// in the future we may try to only generate current version and set
+// `fallback: true` to increase build speed


slow build speed is fine if it optimizes user experience, but if the build times out from memory, we can use that solution for sure.

avatus · 2023-03-15T17:49:42Z

server/paths.ts

+ */
+
+const makeParams = (path?: string) => {
+  const slug = path.split("/").filter((part) => part);


what is the purpose of filter here if we return every element?

The original slug looks like this /var/13.x/page-name/, so we need to filter the first and the last blank segments. We could do it in the slug generation function, but then we will need to add them back to the source map and other places.

avatus · 2023-03-15T17:53:52Z

server/paths.ts

+}
+
+export const getStaticPathsForDocs = (): DocsStaticPath[] => {
+  const result = [];


Suggested change

const result = [];

const result: DocsStaticPath[] = [];

avatus · 2023-03-15T17:55:50Z

server/paths.ts

+    result.push(
+      ...getSlugsForVersion(version, {
+        ignoreLatest: true,
+        withNoIndex: true,
+      }).map(makeParams)
+    );
+
+    if (version === latest) {
+      result.push(...getSlugsForVersion(version).map(makeParams));
+    }


does this mean if version === latest we push the result twice?

Yes, right now, we push twice, but I am thinking about changing it to rewrites in config instead of actual paths.

avatus · 2023-03-15T17:59:51Z

server/paths.ts

+    // Add normalized paths for the current version in additon to full paths
+    if (version === latest) {
+      paths.forEach((path) => {
+        result[normalizeDocSlug(path, version)] = path;
+      });
+    }


does this overwrite a value set in the forEach loop above? We could do a conditional instead

No, it sets values twice. E. g. /docs/ and /docs/ver/12.x/. But I will also try the redirect idea so w may not need it.

avatus · 2023-03-15T18:06:42Z

uvu-tests/rehype-images.test.ts

+    .toString();
+
+const Suite = suite("server/remark-images");
+


awesome tests!

iAdramelk · 2023-03-16T15:51:11Z

@avatus Returned older-versions page and replaced the current version solution with redirects. Please check

iAdramelk · 2023-03-16T16:13:39Z

@avatus ok, we have a problem, lol. I remember why I removed older-versions – we already have it in the docs, so we now have a naming conflict. Will add an exception for now, but we should remove older-versions.mdx from docs.

avatus · 2023-03-16T16:38:45Z

@avatus ok, we have a problem, lol. I remember why I removed older-versions – we already have it in the docs, so we now have a naming conflict. Will add an exception for now, but we should remove older-versions.mdx from docs.

I can remove the older-versions.mdx from the docs folder so it doesn't conflict here. Ill PR and backport it today

avatus · 2023-03-16T16:44:31Z

server/paths.ts

-        };
-      })
+      ...getSlugsForVersion(version)
+        .filter((path) => path !== "/older-versions/") // We should remove older-versions.mdx from docs


We can remove this check once gravitational/teleport#23192 gets pulled in. But good to know we have a killswitch for anything similar in the future. thank you!

avatus

Looks good to me and has been working great locally. Would wait for @alexfornuto to go over it as his comb is much more fine than mine

alexfornuto

The Good

Local refresh - working manually, though it's a bummer that live refresh won't work.
Video auto playing - confirmed.
Images - reviewed several png and svg files, they all appear to render correctly.

The bad

Versioned paths - Using a versioned link for the current version successfully redirects, but using the version switcher on the page errors out:

error - ./content/11.x/docs/pages/deploy-a-cluster/introduction.mdx
Module parse failed: Assigning to rvalue (1:2)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
> ---
| title: "Running a Production Teleport Cluster"
| description: "Guides to running Teleport in production."

This error can't be escaped by navigating to another page, the dev server needs to be restarted.

Vars - now that #254 is merged, can you rebase this branch against master to confirm that it all works when put together?

iAdramelk · 2023-03-16T18:57:02Z

error - ./content/11.x/docs/pages/deploy-a-cluster/introduction.mdx
Module parse failed: Assigning to rvalue (1:2)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders

| title: "Running a Production Teleport Cluster"
| description: "Guides to running Teleport in production."

@alexfornuto Can please you check if you have ver folder with the mdx files inside the pages folder? If yes, can you remove them and try again? They are automatically placed in the folder by the previous build system and can break the new one.

avatus · 2023-03-16T18:58:57Z

FWIW, I had that same versioned pages error until I just deleted the local docs repo and recloned.

This reverts commit 04f39e8.

- Update to Next@13 - Add classes fro images to preserve aspect ratio - Replace probe-image-size with image-size - Move test to other tests location - Move nft excludes to next.config.mjs

iAdramelk · 2023-03-19T18:47:29Z

Ready to merge if @alexfornuto is ok with it.

alexfornuto · 2023-03-20T14:05:00Z

@iAdramelk I am begrudgingly OK with it.

iAdramelk · 2023-03-20T14:08:11Z

@alexfornuto ok, merging it then.

In gravitational/docs#253, we substantially reduced the resource consumption of docs builds. As a result, we can try building the docs as part of the "Lint (Docs)" GitHub Actions workflow in order to prevent build issues from breaking docs deployments. It is currently possible to merge a docs content PR into gravitational/teleport that can later end up breaking deployments of the docs site, e.g., because a video ID is malformed, a code snippet label is unsupported, etc. By building the docs during the lint job, we can prevent this kind of thing from happening. One complication is that the docs engine reads a `config.json` file to match git submodule directories with version of the docs. In the `gravitational/docs` container, `config.json` expects three submodules pointing to three versions of the docs. To get GitHub Actions to build a single docs version, this change overrides the `config.json` file in the gravitational/docs container so it only expects a single version of the docs.

Backports #23464 In gravitational/docs#253, we substantially reduced the resource consumption of docs builds. As a result, we can try building the docs as part of the "Lint (Docs)" GitHub Actions workflow in order to prevent build issues from breaking docs deployments. It is currently possible to merge a docs content PR into gravitational/teleport that can later end up breaking deployments of the docs site, e.g., because a video ID is malformed, a code snippet label is unsupported, etc. By building the docs during the lint job, we can prevent this kind of thing from happening. One complication is that the docs engine reads a `config.json` file to match git submodule directories with version of the docs. In the `gravitational/docs` container, `config.json` expects three submodules pointing to three versions of the docs. To get GitHub Actions to build a single docs version, this change overrides the `config.json` file in the gravitational/docs container so it only expects a single version of the docs.

Backport #23464 In gravitational/docs#253, we substantially reduced the resource consumption of docs builds. As a result, we can try building the docs as part of the "Lint (Docs)" GitHub Actions workflow in order to prevent build issues from breaking docs deployments. It is currently possible to merge a docs content PR into gravitational/teleport that can later end up breaking deployments of the docs site, e.g., because a video ID is malformed, a code snippet label is unsupported, etc. By building the docs during the lint job, we can prevent this kind of thing from happening. One complication is that the docs engine reads a `config.json` file to match git submodule directories with version of the docs. In the `gravitational/docs` container, `config.json` expects three submodules pointing to three versions of the docs. To get GitHub Actions to build a single docs version, this change overrides the `config.json` file in the gravitational/docs container so it only expects a single version of the docs.

Backports #23464 In gravitational/docs#253, we substantially reduced the resource consumption of docs builds. As a result, we can try building the docs as part of the "Lint (Docs)" GitHub Actions workflow in order to prevent build issues from breaking docs deployments. It is currently possible to merge a docs content PR into gravitational/teleport that can later end up breaking deployments of the docs site, e.g., because a video ID is malformed, a code snippet label is unsupported, etc. By building the docs during the lint job, we can prevent this kind of thing from happening. One complication is that the docs engine reads a `config.json` file to match git submodule directories with version of the docs. In the `gravitational/docs` container, `config.json` expects three submodules pointing to three versions of the docs. To get GitHub Actions to build a single docs version, this change overrides the `config.json` file in the gravitational/docs container so it only expects a single version of the docs.

Backport #23464 In gravitational/docs#253, we substantially reduced the resource consumption of docs builds. As a result, we can try building the docs as part of the "Lint (Docs)" GitHub Actions workflow in order to prevent build issues from breaking docs deployments. It is currently possible to merge a docs content PR into gravitational/teleport that can later end up breaking deployments of the docs site, e.g., because a video ID is malformed, a code snippet label is unsupported, etc. By building the docs during the lint job, we can prevent this kind of thing from happening. One complication is that the docs engine reads a `config.json` file to match git submodule directories with version of the docs. In the `gravitational/docs` container, `config.json` expects three submodules pointing to three versions of the docs. To get GitHub Actions to build a single docs version, this change overrides the `config.json` file in the gravitational/docs container so it only expects a single version of the docs.

Backports #23464 In gravitational/docs#253, we substantially reduced the resource consumption of docs builds. As a result, we can try building the docs as part of the "Lint (Docs)" GitHub Actions workflow in order to prevent build issues from breaking docs deployments. It is currently possible to merge a docs content PR into gravitational/teleport that can later end up breaking deployments of the docs site, e.g., because a video ID is malformed, a code snippet label is unsupported, etc. By building the docs during the lint job, we can prevent this kind of thing from happening. One complication is that the docs engine reads a `config.json` file to match git submodule directories with version of the docs. In the `gravitational/docs` container, `config.json` expects three submodules pointing to three versions of the docs. To get GitHub Actions to build a single docs version, this change overrides the `config.json` file in the gravitational/docs container so it only expects a single version of the docs.

vercel bot had a problem deploying to Preview March 15, 2023 13:50 Failure

vercel bot deployed to Preview March 15, 2023 14:16 View deployment

iAdramelk force-pushed the iadramelk/mdx-rehype-v3 branch from e8f505e to 0d71e40 Compare March 15, 2023 14:25

vercel bot deployed to Preview March 15, 2023 14:32 View deployment

iAdramelk marked this pull request as ready for review March 15, 2023 14:54

iAdramelk requested review from alexfornuto and avatus March 15, 2023 14:54

avatus mentioned this pull request Mar 15, 2023

Upgrade to nextjs v13 #237

Closed

avatus reviewed Mar 15, 2023

View reviewed changes

iAdramelk force-pushed the iadramelk/mdx-rehype-v3 branch from 0d71e40 to a57564f Compare March 16, 2023 15:47

iAdramelk requested a review from avatus March 16, 2023 15:49

vercel bot had a problem deploying to Preview March 16, 2023 15:53 Failure

iAdramelk force-pushed the iadramelk/mdx-rehype-v3 branch from a57564f to 6d6e6d5 Compare March 16, 2023 16:16

vercel bot deployed to Preview March 16, 2023 16:22 View deployment

avatus mentioned this pull request Mar 16, 2023

[docs] Remove older-versions from docs gravitational/teleport#23192

Merged

avatus reviewed Mar 16, 2023

View reviewed changes

avatus previously approved these changes Mar 16, 2023

View reviewed changes

alexfornuto requested a review from ptgott March 16, 2023 18:34

alexfornuto suggested changes Mar 16, 2023

View reviewed changes

ptgott mentioned this pull request Dec 11, 2024

Create a manual testing checklist for the docs engine gravitational/docs-website#62

Open

iAdramelk added 4 commits March 17, 2023 04:00

Revert "Revert "Second version of mdx-to-hast transformer" (#241)"

19e92d6

This reverts commit 04f39e8.

Disable pages cache in dev mode

ab7f7a6

Fix boolean attributes on HTML nodes and add tests

14245b3

Update to Next@13 to fix issue with images

f4c1323

- Update to Next@13 - Add classes fro images to preserve aspect ratio - Replace probe-image-size with image-size - Move test to other tests location - Move nft excludes to next.config.mjs

iAdramelk merged commit 5821d58 into main Mar 20, 2023

iAdramelk deleted the iadramelk/mdx-rehype-v3 branch March 20, 2023 14:08

ptgott mentioned this pull request Mar 22, 2023

Attempt to build the docs in "Lint (Docs)" gravitational/teleport#23464

Merged

ptgott mentioned this pull request Mar 23, 2023

[v12] Attempt to build the docs in "Lint (Docs)" gravitational/teleport#23530

Merged

ptgott mentioned this pull request Mar 23, 2023

[v11] Attempt to build the docs in "Lint (Docs)" gravitational/teleport#23531

Merged

ptgott mentioned this pull request Mar 23, 2023

[v10] Attempt to build the docs in "Lint (Docs)" gravitational/teleport#23532

Merged

ptgott mentioned this pull request May 19, 2023

Live Refresh is broken in dev environments. #247

Closed

Conversation

iAdramelk commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iAdramelk commented Mar 16, 2023

Uh oh!

iAdramelk commented Mar 16, 2023

Uh oh!

avatus commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus left a comment

Choose a reason for hiding this comment

Uh oh!

alexfornuto left a comment

Choose a reason for hiding this comment

Uh oh!

iAdramelk commented Mar 16, 2023

Uh oh!

avatus commented Mar 16, 2023

Uh oh!

iAdramelk commented Mar 19, 2023

Uh oh!

alexfornuto commented Mar 20, 2023

Uh oh!

iAdramelk commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iAdramelk commented Mar 15, 2023 •

edited

Loading

vercel bot commented Mar 15, 2023 •

edited

Loading

avatus commented Mar 16, 2023 •

edited

Loading