-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
refactor(gatsby): Convert inference metadata to TypeScript #23264
refactor(gatsby): Convert inference metadata to TypeScript #23264
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.
Thank you for this PR! I've left several comments inline.
Also I think it makes sense to remove those comments (as they are now actually represented as types):
gatsby/packages/gatsby/src/schema/infer/inference-metadata.js
Lines 49 to 112 in 4043129
### Metadata structure | |
```javascript | |
type TypeMetadata = { | |
total?: number, | |
ignored?: boolean, | |
ignoredFields?: Set<string>, | |
fieldMap?: { [string]: ValueDescriptor }, | |
typeName?: string, | |
dirty?: boolean, // tracks structural changes only | |
disabled?: boolean, | |
} | |
type Count = number | |
type NodeId = string | |
type ValueDescriptor = { | |
int?: TypeInfoNumber, | |
float?: TypeInfoNumber, | |
date?: TypeInfoDate, | |
string?: TypeInfoString, | |
boolean?: TypeInfoBoolean, | |
array?: TypeInfoArray, | |
relatedNode?: TypeInfoRelatedNode, | |
relatedNodeList?: TypeInfoRelatedNode, | |
object?: TypeInfoObject, | |
} | |
abstract type TypeInfo = { | |
first?: NodeId | void, // Set to undefined if "del" | |
total: Count, | |
} | |
type TypeInfoString = TypeInfo & { | |
empty: Count, | |
example: string, | |
} | |
type TypeInfoDate = TypeInfo & { | |
example: string, | |
} | |
type TypeInfoNumber = { | |
example: number, | |
} | |
type TypeInfoBoolean = { | |
example: boolean, | |
} | |
// "dprops" is "descriptor props", which makes it easier to search for than "props" | |
type TypeInfoObject = TypeInfo & { | |
dprops?: {[name: "number" | "string" | "boolean" | "null" | "date" | "string" | "array" | "object"]: Descriptor}, | |
} | |
type TypeInfoArray = TypeInfo & { | |
item: ValueDescriptor, | |
} | |
type TypeInfoRelatedNode = TypeInfo & { | |
nodes: { [NodeId]: Count } | |
} | |
``` |
Thanks, @vladar. It's an interesting challenge migrating a codebase you don't know, and trying to not break it! 😆 I'll get on those comments now, and try to keep it to logical commits. I'll probably edit the code comments at the top of the file last, if that's okay, so I can take a pass at rewriting if needs be but referring to new interface names. |
@chooban Sure, you are doing a great job here! Special kudos for making it easy for review 🏆 |
After tightening the original util function to return a string only, it seemed sensible to convert that to use nullish coalescing. At that point, seeing the function is only used in a single place and not exported, I inlined it.
if (typeInfo === undefined) { | ||
typeInfo = descriptor[typeName] = { total: 0 } | ||
typeInfo = (descriptor[typeName] as ITypeInfo) = { total: 0 } |
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.
There's too much teling the compiler what to do here, but otherwise it complains about the lack of a value for empty
in the case it's a string field. I could add a small factory function that returns an ITypeInfo
matching object based on the typeName
. Is that extra code to maintain worth it compared to the quick compiler hint that's covered by the tests?
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.
Yeah, I think it's OK
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.
OK to add the factory function, or to leave it alone?
I've no idea why I've got failing tests now. I can't seem to get master to pass either. :/
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.
Leave it as is for now. As for tests - there were some unrelated issues recently. Should be fixed now. Let's wait a bit when they finish.
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.
OK, I think we are good to go. Ignore failing www
tests as those should be fixed in master now. Thanks for the PR! 💜
On to the next one! |
* fix(gatsby): Throw error on default export in gatsby-ssr/brows… (#23133) * fix: Handle default export case * heh * fix comment * chore(release): Publish - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] * fix(docs): Using-MDX link (#23288) * fix(docs): Link to apollo blog (#23290) * fix(docs): change gatsby-config mention for global CSS import (#23245) * fix `gatsby-config` mention * chore: format Co-authored-by: gatsbybot <[email protected]> * docs: added details signifying that node fields can be arbitra… (#23106) * added details signifying that node fields can be arbitrary * Update docs/docs/data-fetching.md Co-Authored-By: Kyle Gill <[email protected]> * Update docs/docs/data-fetching.md Co-Authored-By: Kyle Gill <[email protected]> * removed empty line to fix failed linting tests Co-Authored-By: Kyle Gill <[email protected]> Co-authored-by: Kyle Gill <[email protected]> * chore(gatsby): drop the the (#23295) * chore(showcase): Add new sites built by Bejamas to showcase; c… (#23125) * chore(sites): add new sites built by bejamas * chore(creators): update bejamas cover * chore(showcase):Add SofaScore Corporate web to showcase sites (#23128) Co-authored-by: Mario Nikolaus <[email protected]> * fix local links (#23149) * chore(docs): Reference new ComponentModel component rather than generic LayerModel (#23241) * Reference new ComponentModel component rather than generic LayerModel * Remove use of t macro from layer models * Half-fixing lingui code * Revert "Half-fixing lingui code" This reverts commit 64f052a. * Remove Lingui from layer-model * fix(www): fix sidebar (#23301) * fix(www): fix sidebar * Update sidebar.js * fix(recipes): Fix shadow-file resource for scoped NPM packages & re-enable e2e testing for resources (#23274) * fix(recipes): Fix shadow-file resource for scoped NPM packages * WILL SLASH FIX WINDOWS??? * Add missing code block * Try again * hmmm joinPath maybe is the problem * More fixes * try reenabling tests * disable colors in diffs * Fix npm script test * Why do these keep changing all the time?? * Use parsed value for the plan * Disable Chalk colors on unit tests so match local ones * Update packages/gatsby-recipes/src/providers/utils/get-diff.js Co-Authored-By: John Otander <[email protected]> Co-authored-by: John Otander <[email protected]> Co-authored-by: gatsbybot <[email protected]> * chore(release): Publish - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] * chore(starters): add gatsby-starter-ecommerce (#23037) * chore(starters): add gatsby-starter-ecommerce * chore(starters): add gatsby-starter-ecommerce Co-authored-by: gatsbybot <[email protected]> * Add site Interficie Internet Services (#23130) * Add site Interficie Internet Services * Correct JavaScript cammelcase * Prettier format Co-authored-by: Obinna Ekwuno <[email protected]> Co-authored-by: gatsbybot <[email protected]> * fix brand names (#23142) * fix(blog): remove local domain (#23141) * fix remove local domain * fix local domain * fix local domain * fix remove local domain * fix for linter * Create paul scanlon 100 days of Gatsby blog post and add assets (#23294) * Create paul scanlon 100 days of Gatsby blog post and add assets * fix frontmatter * fix frontmatter issue and ad relative links * fix typos and spacing issues * Update docs/blog/2020-04-20-paulie-scanlons-journey-of-100-days/index.md Co-Authored-By: LB <[email protected]> Co-authored-by: Laurie Barth <[email protected]> Co-authored-by: LB <[email protected]> * Clarify prefix-paths variable. (#23309) The line (one line, two places) I'm editing was copied from a comment in #21627 without the surrounding context. This edit provides that context. Co-authored-by: gatsbybot <[email protected]> * chore(showcase):Add shreysachdeva.tech to showchase (#23032) * Add shreysachdeva.tech to sites.yml **Description** Added the details of my personal website shreysachdeva.tech. **Documentation** No documentation **Related Issues** N/A * chore: format Co-authored-by: gatsbybot <[email protected]> Co-authored-by: Obinna Ekwuno <[email protected]> * fix(www): Fix sidebar expansion behavior (#23325) * [gatsby-plugin-manifest] Generate a 32x32 favicon instead of generating from first manifest icon (#23077) * Decouple favicons from manifest icons * Update packages/gatsby-plugin-manifest/README.md * Replace include_favicon undefined check with nullish coalescing operator Co-Authored-By: Alex Moon <[email protected]> * Assert gatsby-plugin-manifest generates favicon PNG (unless include_favicon option is false) * Don't mock createContentDigest in gatsby-plugin-manifest's gatsby-ssr tests (to be consistent with gatsby-node tests) * Tweak a comment Co-authored-by: LB <[email protected]> Co-authored-by: Alex Moon <[email protected]> * Use mutations in Gatsby Admin * Remove stray console.logs * chore(gatsby-recipes): Update README typo (#23333) Fixed typo in README.md * fix(gatsby): Check for files before delete action when using GATSBY_EXPERIMENTAL_PAGE_BUILD_ON_DATA_CHANGES (#23219) * Check for files before delete action * fix lint issues * fix lint issues and add return type to checkFolderHasContent * rename check function Co-authored-by: gatsbybot <[email protected]> * Install plugins as prod dependencies * chore(release): Publish - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] * Add Besoegsvenner site to showcase (#23172) * Add Besoegsvenner site to showcase 50.000 elderly people in Denmark feel lonely. This project seeks to inform people to become visitor friends ("Besøgsven" in Danish) to help fight loneliness and bring new friendships in to the world. * Update sites.yml Co-authored-by: Obinna Ekwuno <[email protected]> Co-authored-by: gatsbybot <[email protected]> * chore(benchmarks): Add Drupal data updater script to update benchmark data (#23242) * Add Drupal data updater script to update benchmark data * Remove console.log statement Co-authored-by: gatsbybot <[email protected]> * Make TypeScript happy * fix(gatsby): fix Uncaught TypeError in navigation.js (#23196) * Add null check to address #21263 #21263 is an issue many people are arriving at for a variety of reasons. Since the code contains this logic on the following line: ``` if (pageHeadings && pageHeadings.length) { ``` There should be no issue in seeing if `document.getElementById(`gatsby-focus-wrapper`) exists first before attempting to look up `getElementsByTagName`. * Update packages/gatsby/cache-dir/navigation.js Co-Authored-By: Vladimir Razuvaev <[email protected]> * chore: format Co-authored-by: Vladimir Razuvaev <[email protected]> Co-authored-by: gatsbybot <[email protected]> * Add support for file argument in remark plugins for gatsby-plugin-mdx (#21489) * chore(gatsby-cli) Migrate remaining files in reporters/logger/ink to Typscript (#22782) * Migrate remaining files in gatsby-cli/reporters/logger/ink to TS * Rename "IProps" to something more contextual Extend interface parameter individually instead of using utility * Clean up after merging the redux master changes Co-authored-by: Blaine Kasten <[email protected]> * Fix Drupal data update script (#23349) * Add noop data update scripts for all benchmarks (#23352) * Add noop data update scripts for all CMS benchmarks * Add data update to remainder of benchmarks * chore(babel-preset-gatsby): Convert dependencies to TS (#22038) Co-authored-by: Blaine Kasten <[email protected]> * fix(gatsby): Improve error message when calling useStaticQuery without graphql (#23189) * Add warning message for a long queries (#23261) * Add warning message for a long queries * Change timeout to 15 seconds * Replace console with report * Print out queryJob.componentPath instead of page.internalComponentName * Add new info in the warning message Co-authored-by: gatsbybot <[email protected]> * refactor(gatsby): Convert inference metadata to TypeScript (#23264) * refactor(inference-metadata): Convert to TypeScript * Remove unused variable * Forgot some types * refactor(type-conflict-reporter): Node description After tightening the original util function to return a string only, it seemed sensible to convert that to use nullish coalescing. At that point, seeing the function is only used in a single place and not exported, I inlined it. * refactor(inference-metadata): Interface renaming * fix: Extract ValueType type * refactor: Change type from unknown to object * Corrected types, but left the code alone * Removed redundant comments * Update README.md (#23357) * feat(gatsby-remark-images): support markdownCaptions in mdx + fix for remark (#21188) * pass compiler to gatsby-remark-* plugins in the mdx plugin * make getImageCaption async * use async mdx compiler * fix for using compiler * don't mutate the node multiple times * convert markdown ast to html ast Co-authored-by: vladar * chore(release): Publish - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] - [email protected] * Fix lint errors * Fix TypeScript unused declaration error * Fix linting Co-authored-by: Lennart <[email protected]> Co-authored-by: Jeremy Albright <[email protected]> Co-authored-by: gatsbybot <[email protected]> Co-authored-by: Elijah <[email protected]> Co-authored-by: Kyle Gill <[email protected]> Co-authored-by: Peter van der Zee <[email protected]> Co-authored-by: Michał Zieliński <[email protected]> Co-authored-by: Mario Nikolaus <[email protected]> Co-authored-by: Mario Nikolaus <[email protected]> Co-authored-by: Michael <[email protected]> Co-authored-by: Aisha Blake <[email protected]> Co-authored-by: Ward Peeters <[email protected]> Co-authored-by: Kyle Mathews <[email protected]> Co-authored-by: John Otander <[email protected]> Co-authored-by: Lorenzo GM <[email protected]> Co-authored-by: aperacaula <[email protected]> Co-authored-by: Obinna Ekwuno <[email protected]> Co-authored-by: Michelle Gienow <[email protected]> Co-authored-by: Laurie Barth <[email protected]> Co-authored-by: LB <[email protected]> Co-authored-by: Alex Fornuto <[email protected]> Co-authored-by: Shrey Sachdeva <[email protected]> Co-authored-by: Nat Alison <[email protected]> Co-authored-by: Owen <[email protected]> Co-authored-by: Alex Moon <[email protected]> Co-authored-by: Marcus Lyons <[email protected]> Co-authored-by: Stuart Rayson <[email protected]> Co-authored-by: Michal Piechowiak <[email protected]> Co-authored-by: Jacob Packert <[email protected]> Co-authored-by: Shane Thomas <[email protected]> Co-authored-by: nick shook <[email protected]> Co-authored-by: Vladimir Razuvaev <[email protected]> Co-authored-by: Kai Hao <[email protected]> Co-authored-by: Arthur <[email protected]> Co-authored-by: Blaine Kasten <[email protected]> Co-authored-by: Nicholas Duffy <[email protected]> Co-authored-by: Michaël De Boey <[email protected]> Co-authored-by: Rebenkov Aleksey <[email protected]> Co-authored-by: Ross Hendry <[email protected]> Co-authored-by: Khaled Garbaya <[email protected]> Co-authored-by: Mathieu Dutour <[email protected]> Co-authored-by: Sidhartha Chatterjee <[email protected]>
Description
I took a more sensible approach than with #23093; I switched off the linting errors about use before define so that an easier side-by-side comparison was possible. I also did the conflict reporter class as migrating the test highlighted the need for it on the IMetadata interface.
I was lax on the types in the test, as some of the input had very little crossover with what I think is the correct Node type. That meant there was a lot of
as Node
and I had to pass it through asas unknown
as well. So there's a minimum of change in the contents of the tests, but I'm not going to say "Wow! Isn't this a wonderful new paradigm!".References
ref #21995