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

feat: rdme docs upload #1159

Merged
merged 60 commits into from
Feb 4, 2025
Merged

feat: rdme docs upload #1159

merged 60 commits into from
Feb 4, 2025

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Jan 31, 2025

🚥 Resolves RM-11796, RM-11797

🧰 Changes

🚧 outstanding work

🔜 to be addressed in follow-up PRs

  • safeguards for users that use bidirectional syncing (ticketed in RM-11883 and RM-11901)
  • updating the v9 migration guide + deprecation warnings (ticketed in RM-11902)

things to call out

  • on a high level, we're mostly matching the functionality of v9's rdme docs in that we're taking markdown files and sending their contents as JSON payloads to the ReadMe API. inferring page hierarchy from directory/file structure is out of scope for this work.
  • one big change from rdme@9 is that we now validate page frontmatter against the schemas from our OAS before making any API requests and prompt the user to migrate any frontmatter that we recognize as legacy APIv1 attributes. i'm open to reevaluating this paradigm, but i think it'll be helpful for folks migrating. as an interim escape hatch, this command has a hidden --skip-validation flag, just in case.
  • our API expects category and parent page URIs to be a full URI (e.g., /versions/stable/categories/guides/some-uri), but that's a bit arduous for rdme users since most of the URI can be inferred from the rdme command context. as a result, rdme will automatically expand any category/parent URIs that don't match that pattern (e.g., a category URI frontmatter value of some-uri will be changed to /versions/stable/categories/guides/some-uri before making the POST/PATCH request. the frontmatter will still say some-uri.)
  • the vast majority of this diff is tests + fixtures + our OAS. the three files of note are:
    • src/lib/syncPagePath.ts
    • src/lib/frontmatter.ts
    • src/commands/docs/upload.ts
  • i've tried to build this in a testable and robust way where we can easily add support for reference pages, custom pages, etc. in the future
  • PATCH requests to our API (i.e., for making updates to guide pages) that include slug in the body payload will always rename the page. so if you have a slug called some-page and your body payload contains some-page, the API will change your slug to some-page-1. this is most likely unexpected behavior for users of rdme, so i'm always removing the slug from the body payload in favor of using the URL instead. the trade-off is that page renaming isn't really a thing via rdme, but that's why we support bidirectional syncing, right?

🧬 QA & Testing

we're at nearly 100% test coverage with this work 📈 to QA it yourself, check out this branch and run the following:

# install + set up 
npm ci
npm run build
# grab an API key from a readme-refactored-enabled project and pop it at the end of this command:
bin/run.js docs upload __tests__/__fixtures__/docs/mixed-docs --key <key>

kanadgupta added a commit that referenced this pull request Jan 31, 2025
## 🧰 Changes

cherry-picking over a few random refactors from
#1159 that are mostly unrelated to
the PR itself:

- [x] removes unused fixtures
- [x] smol copy tweaks for changelog command ("syncing" is reserved for
bidi, this is "uploading"
- [x] jsdoc tweaks, including deprecating APIv1-related cruft
- [x] various type enhancements
- [x] renaming a few things
- [x] adding `oclif`'s `this` context (so we can simplify functions and
add better debug logs)

## 🧬 QA & Testing

do tests still pass?
@kanadgupta kanadgupta requested a review from a team January 31, 2025 17:34
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

leaving formal approval off until this is ready for review but aside from a couple minor questions this looks great. awesome job.

__tests__/commands/docs/upload.test.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
__tests__/commands/docs/upload.test.ts Outdated Show resolved Hide resolved
__tests__/lib/frontmatter.test.ts Show resolved Hide resolved
src/commands/docs/upload.ts Outdated Show resolved Hide resolved
src/lib/frontmatter.ts Outdated Show resolved Hide resolved
src/lib/readmeAPIFetch.ts Show resolved Hide resolved
src/lib/syncPagePath.ts Show resolved Hide resolved
src/lib/syncPagePath.ts Show resolved Hide resolved
src/lib/syncPagePath.ts Outdated Show resolved Hide resolved
kanadgupta added a commit that referenced this pull request Jan 31, 2025
## 🧰 Changes

chunks out a lot of PR feedback and random changes from #1159:

- [x] renames all "front matter" to "frontmatter"
- [x] better `nock` and `fs` setup/teardown
- [x] cherry-picking over
c871520
- [x] various cleanups
- [x] bumps deps:

before:

```Package                    Current   Wanted   Latest  Location                                Depended by
@readme/better-ajv-errors    2.0.0    2.1.2    2.1.2  node_modules/@readme/better-ajv-errors  rdme
eslint                      8.57.1   8.57.1   9.19.0  node_modules/eslint                     rdme
oclif                      4.17.20  4.17.21  4.17.21  node_modules/oclif                      rdme
semver                       7.6.3    7.7.0    7.7.0  node_modules/semver                     rdme
undici                      5.28.5   5.28.5    7.3.0  node_modules/undici                     rdme
```

after:

```
Package  Current  Wanted  Latest  Location             Depended by
eslint    8.57.1  8.57.1  9.19.0  node_modules/eslint  rdme
undici    5.28.5  5.28.5   7.3.0  node_modules/undici  rdme
```

## 🧬 QA & Testing

do tests still pass?

---------

Co-authored-by: Jon Ursenbach <[email protected]>
| 🚥 Resolves RM-11800 |
| :------------------- |

## 🧰 Changes

first (and hopefully totally adequate!) pass at the `rdme docs`
migration guide. we can continue to iterate after this initial beta is
out, based on feedback!

## 🧬 QA & Testing

does this language all look sound to you? any big points that we're
missing?
@kanadgupta kanadgupta added command:docs Issues pertaining to the `docs`, `changelogs`, or `custompages` commands enhancement New feature or request documentation Improvements or additions to documentation labels Feb 4, 2025
@kanadgupta kanadgupta marked this pull request as ready for review February 4, 2025 00:29
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

mildly concerned about some file encoding bullshit but it's nothing that can't wait.

this all looks awesome.

});
});

describe('#fetchSchema', () => {
Copy link
Member

Choose a reason for hiding this comment

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

May want to move this test case to a different test file because fetchSchema is part of readmeAPIFetch, not frontmatter.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah good catch, ty

src/lib/frontmatter.ts Show resolved Hide resolved
return this.readmeAPIFetch(
route,
{ method: 'POST', headers, body: JSON.stringify(payload) },
{ filePath, fileType: 'path' },
Copy link
Member

Choose a reason for hiding this comment

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

What's this third argument for?

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:

if (filePath) {
/**
* Constructs a full URL to the file using GitHub Actions runner variables
* @see {@link https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables}
* @example https://github.com/readmeio/rdme/blob/cb4129d5c7b51ff3b50f933a9c7d0c3d0d33d62c/documentation/rdme.md
*/
try {
const sourceUrl = new URL(
`${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/blob/${process.env.GITHUB_SHA}/${filePath}`,
).href;
headers.set('x-readme-source-url', sourceUrl);
} catch (e) {
this.debug(`error constructing github source url: ${e.message}`);
}
}

@kanadgupta kanadgupta merged commit 69f9c7e into next Feb 4, 2025
8 checks passed
@kanadgupta kanadgupta deleted the kanad-2025-01-14/docs-v2 branch February 4, 2025 01:23
kanadgupta pushed a commit that referenced this pull request Feb 4, 2025
# [10.2.0-next.1](v10.1.2-next.1...v10.2.0-next.1) (2025-02-04)

### Features

* `rdme docs upload` ([#1159](#1159)) ([69f9c7e](69f9c7e))

[skip ci]
@kanadgupta
Copy link
Member Author

🎉 This PR is included in version 10.2.0-next.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:docs Issues pertaining to the `docs`, `changelogs`, or `custompages` commands documentation Improvements or additions to documentation enhancement New feature or request released on @next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants