-
Notifications
You must be signed in to change notification settings - Fork 203
fix: Handle LiteralUnion types correctly in documenter for CodeEditor language property #4009
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4009 +/- ##
==========================================
+ Coverage 97.17% 97.24% +0.07%
==========================================
Files 856 859 +3
Lines 25241 25389 +148
Branches 8938 9051 +113
==========================================
+ Hits 24527 24690 +163
+ Misses 708 652 -56
- Partials 6 47 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gethinwebster
left a comment
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 think that the current behavior is correct/expected. And so this should probably be handled in the website instead of the documenter. Or, maybe the documenter needs to create two different types of unions: string-union and literal-union or something like that. With the current proposed change, we're losing information about the underlying types
| "inlineType": { | ||
| "name": "InputProps.Step", | ||
| "type": "union", | ||
| "values": [ |
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.
Based on this example, I'm not sure the previous behavior was actually incorrect. In this case, the correct type is number | "any", which is no longer preserved correctly.
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've updated the patch to be more targeted, it now only treats true LiteralUnion
patterns (those with the { _?: never } marker) as string unions, while preserving mixed unions like number | 'any' with their full type information.
| @@ -0,0 +1,40 @@ | |||
| diff --git a/node_modules/@cloudscape-design/documenter/components/object-definition.js b/node_modules/@cloudscape-design/documenter/components/object-definition.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.
I guess this file shouldn't be here?
| "xl", | ||
| "xxl", | ||
| "xxxl", | ||
| "BoxProps.Spacing", |
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 one is still incorrect: "s", "m" etc. are strings, BoxProps.Spacing is a type, but all look the same in the new output
Description
Fixed an issue where
CodeEditorProps.Languagetype values were incorrectly formatted in generated component definitions. TheLiteralUnion<BuiltInLanguage, string>type was not properly recognized as a primitive string type, causing language values to appear as"\"javascript\""instead of"javascript"in the documentation.Related links, issue #, if available:
AWSUI-61340How has this been tested?
src/code-editor/__tests__/language-selector.test.tsx'javascript') display correctly'partiql') work with labelsLiteralUniontype assignments work without TypeScript errorslib/components-definitions/components/code-editor.jsnow generates clean valuesReview checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.