-
Notifications
You must be signed in to change notification settings - Fork 558
ConfigMap related cleanup and refactors #25868
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
base: main
Are you sure you want to change the base?
Conversation
| // This is a bad cast away from Partial that getConfigsForCompatMode provides. | ||
| // ConfigMap should be restructured to provide RuntimeOptionsAffectingDocSchema guarantee. | ||
| ) as RuntimeOptionsAffectingDocSchema; |
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.
Removing this cast is possible now because getConfigsForMinVersionForCollab can no longer return undefined for entries, which if returned here would have been unsupported in RuntimeOptionsAffectingDocSchema.
This code is now much more type safe since we no longer have a cast that could hide typing errors, and we no longer have the risk of undefined values slipping through as the FlushMode if the config changes or the collab version is lower than the lowest entry for them in the config map.
| [undefined, "1.0.0"], | ||
| [true, "2.40.0"], | ||
| ]), | ||
| } as const satisfies ConfigValidationMap<RuntimeOptionsAffectingDocSchema>; |
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.
using satisfies instead of the variable type here doesn't help anything, so I have changed it to the simpler more conventional pattern. This also likely improves error messages.
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.
Same for runtimeOptionsAffectingDocSchemaConfigMap above
| // Iterate over configMap to get default values for each option. | ||
| for (const key of Object.keys(configMap)) { | ||
| // Type assertion is safe as key comes from Object.keys(configMap) | ||
| const config = configMap[key as keyof T]; |
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 refactor removes this lookup and related type cast: this and the removal of the else branch within the loop were the main things getting cleaned up here.
| const config = configMap[key as keyof T]; | ||
| // Sort the versions in ascending order so we can short circuit the loop. | ||
| const versions = Object.keys(config).sort(compare); | ||
| for (const [key, config] of Object.entries(configMap)) { |
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.
A future follow-up change will factory the body of this loop into a reusable function so this logic can be run on a single config map entry. SharedTree has use-cases for this, like codec write format selection which would be nice to be able to do where needed and not as part of a larger config map.
Description
This adjust several ConfigMap related internal APIs and behaviors, making the code more strict about input and outputs.
Reviewer Guidance
The review process is outlined on this wiki page.