refactor(docs-page): to TS; split up file #360
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hashicorp/react-components/6miKUkyNtmdtK3bYeA2sgwHiLvj3 |
|
packages/docs-page/server/consts.ts
Outdated
| export const ENABLE_VERSIONED_DOCS = process.env.ENABLE_VERSIONED_DOCS | ||
| export const VERCEL_ENV = process.env.VERCEL_ENV |
There was a problem hiding this comment.
nit: I'm not super crazy about re-assigning env vars in a separate file like this. I find it harder to grok for usage and it's just another layer of indirection.
That said, I'm not willing to die on this hill and if you feel this is a better structure I'm okay with that. 😄
There was a problem hiding this comment.
I'm not tied to this! Happy to revert this change...
I'm more interested in lifting up environment variable usage into function args, so that the functions can be more pure. Didn't get to this part yet though...
There was a problem hiding this comment.
Something like
- import { ENABLE_VERSIONED_DOCS, VERCEL_ENV, DEFAULT_PARAM_ID } from './consts'
+ import { DEFAULT_PARAM_ID } from './consts' Then in a function
- function generateStaticPaths(context) { ... }
+ const options = {
+ VERCEL_ENV: process.env.VERCEL_ENV,
+ ENABLE_VERSIONED_DOCS: process.env.ENABLE_VERSIONED_DOCS,
+ }
+
+ function generateStaticPaths(context, { VERCEL_ENV, ENABLE_VERSIONED_DOCS } = options) { ... }There was a problem hiding this comment.
Updating this now... and removing the env vars from consts.ts
| versions = versionMetadataList.map((e) => { | ||
| const { isLatest, version, display } = e | ||
| if (isLatest) { | ||
| latestVersion = version | ||
| } | ||
|
|
||
| const displayValue = display ?? version | ||
|
|
||
| return { | ||
| name: isLatest ? 'latest' : version, | ||
| label: isLatest ? `${displayValue} (latest)` : displayValue, | ||
| } | ||
| }) |
There was a problem hiding this comment.
seems like this could be a fair candidate to extract into a function that's easier to test
There was a problem hiding this comment.
extracted into mapVersionList
Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>
thiskevinwang
changed the title
- refactor `generateStaticProps` logic
|
Looking good! Could you test this with a canary release in a site? That would be full validation that the refactor is good, apart from your tests 😄 |
thiskevinwang
added
the
release:canary
📦 Canary Packages PublishedLatest commit: cd07f61 Published 1 packages@hashicorp/react-docs-page@14.2.4-canary-202183015355 |
thiskevinwang
changed the title
|
Merging into #354 — will continue UAT there! |
* refactor(docs-page): to TS; split up file * test: add at least snapshot tests for all files * chore: fixtures * chore: update snapshots * chore: rename to moizeOpts * Update packages/docs-page/server/generate-static-props.ts Co-authored-by: Bryce Kalow <bkalow@hashicorp.com> * chore: extract `mapVersionList`; - refactor `generateStaticProps` logic * chore: `generateStaticProps` snapshot * chore: remove env vars from `consts.ts` Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>
* refactor(docs-page): to TS; split up file * test: add at least snapshot tests for all files * chore: fixtures * chore: update snapshots * chore: rename to moizeOpts * Update packages/docs-page/server/generate-static-props.ts Co-authored-by: Bryce Kalow <bkalow@hashicorp.com> * chore: extract `mapVersionList`; - refactor `generateStaticProps` logic * chore: `generateStaticProps` snapshot * chore: remove env vars from `consts.ts` Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>
* refactor(docs-page): to TS; split up file * test: add at least snapshot tests for all files * chore: fixtures * chore: update snapshots * chore: rename to moizeOpts * Update packages/docs-page/server/generate-static-props.ts Co-authored-by: Bryce Kalow <bkalow@hashicorp.com> * chore: extract `mapVersionList`; - refactor `generateStaticProps` logic * chore: `generateStaticProps` snapshot * chore: remove env vars from `consts.ts` Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>
* feat(docs-page): server.js calls content api * chore: add logging to generateStaticPaths * feat: use display attribute if available * fix: only use display for option label * feat: always fetch remote content in production * fix: `moize` usage * refactor(docs-page/server): to TS; split up files (#360) * refactor(docs-page): to TS; split up file * chore: extract `mapVersionList`; - refactor `generateStaticProps` logic Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>
asana
Description
docs-page/server.jsinto isolated TypeScript files.moizeusageThis is meant to merge into: