Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
generateApiComponents.ts
and tests #1074Add
generateApiComponents.ts
and tests #1074Changes from 4 commits
ce2f582
dc0f12f
5d14f12
77a99f3
1118c9a
9c5e815
27cffc9
069585c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Not a big deal if this is going to be annoying to implement, but you can switch processHtml.md to use this now, along with
findByText
. That way we don't duplicate. And you could move the tests too if it's too hard.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.
Actually, it probably would be good to do this if isn't too much of a pain because it may take a few days to land the follow up PR since it's blocked by the Docker image being updated for staging.
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.
Done!
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
flatMap
surprises me becauserawSignature
is a string. Should this beAlso can
extraRawSignatures
have a more precise type likestring[]
orArray<string | undefined>
? Seems weird to have all the ComponentProps if we're going to discard them.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.
In reality,
rawSignature
is astring
but could beundefined
given that we have API types without it. The problem with using thefilter
is that we get the type(string | undefined)[]
instead ofstring[] | undefined
as in theflatMap
.This is a good idea. I think we could remove the
flatMap
or themap
+filter
entirely, but I would like to do it in #1026 as a follow-up because we can simplify the main function by only storing the overloaded signature and not all props. The functionaddExtraSignatures
will make all the work as a black boxThere 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.
Why is
id
an empty string?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's empty because it's not needed, but I was able to remove the span and the paragraph too 👍 . Apparently, we only need to add the code tag. Without the code tag, the signature has an extra apostrophe at the beginning and at the end