-
Notifications
You must be signed in to change notification settings - Fork 512
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
fix(types): pass types through memoize properly #10567
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, just skimmed over, and left some comments.
kumascript/index.ts
Outdated
@@ -37,7 +37,7 @@ export async function render( | |||
selective_mode = false, | |||
invalidateCache = false, | |||
}: RenderOptions = {}, | |||
doc?: Doc | |||
blogPost?: BlogPostDoc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. Why would kumascript.render()
return a BlogPostDoc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, this is the last argument, the return type is the line below (the Promise)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should stay called doc
it just replaces the Document.findByURL
. And the type is tricky. Probably Doc
as BlogPostDoc
should just extend Doc
but that is utterly broken right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Doc
applies here, as it has many more fields/requirements compared to BlogPostDoc
.
BlogPostDoc:
Lines 347 to 355 in 006eb85
interface BlogPostDoc { | |
url: string; | |
rawBody: string; | |
metadata: BlogPostMetadata & { locale: string }; | |
isMarkdown: boolean; | |
fileInfo: { | |
path: string; | |
}; | |
} |
DocMetadata/Doc:
Lines 128 to 159 in 006eb85
export interface DocMetadata { | |
title: string; | |
short_title: string; | |
locale: string; | |
native: string; | |
pageTitle: string; | |
mdn_url: string; | |
related_content: any[]; | |
modified: string; | |
flaws: Flaws; | |
other_translations?: Translation[]; | |
parents?: DocParent[]; | |
source: Source; | |
contributors: string[]; | |
isTranslated: boolean; | |
isActive: boolean; | |
hasMathML?: boolean; | |
isMarkdown: boolean; | |
summary: string; | |
popularity?: number; // Used for search. | |
noIndexing?: boolean; | |
browserCompat?: string[]; | |
baseline?: SupportStatus; | |
hash?: string; | |
} | |
export interface Doc extends DocMetadata { | |
sidebarHTML: string; | |
sidebarMacro?: string; | |
toc: Toc[]; | |
body: Section[]; | |
} |
kumascript/index.ts
Outdated
@@ -14,7 +14,7 @@ import { | |||
LIVE_SAMPLES_BASE_URL, | |||
} from "../libs/env/index.js"; | |||
import { SourceCodeError } from "./src/errors.js"; | |||
import { Doc } from "../libs/types/document.js"; | |||
import { BlogPostDoc } from "../build/blog.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw should BlogPostDoc
be moved into libs/types
?
content/translations.ts
Outdated
@@ -8,7 +8,7 @@ const LANGUAGES = new Map( | |||
}) | |||
); | |||
|
|||
type Translation = { | |||
export type Translation = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace the type declaration here, as there was a duplicated one in libs/types/document.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed in c00822d.
This pull request has merge conflicts that must be resolved before it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeoMcA Please have a look at the changes I pushed, but LGTM.
Summary
Problem
While reviewing #10433 it annoyed me that
memoize
wasn't passing types through properly, and we were losing types in most ofbuild/curriculum.ts
because of this.Solution
Pass through the types of the wrapped function in
memoize
, fix the things this breaks. (I suggest reviewing with ignore whitespace because one function gets majorly indented from these fixes).How did you test this change?
yarn check:tsc