fix: undefined response from get-github-info call #510
Conversation
|
Hooray! All contributors have signed the CLA. |
🦋 Changeset detectedLatest commit: 9f123e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tuanddd
force-pushed
the
fix/invalid-repo-name
branch
from
2835864
to
2cf1f8b
Compare
| @@ -2,6 +2,8 @@ | |||
| import fetch from "node-fetch"; | |||
| import DataLoader from "dataloader"; | |||
|
|
|||
| const validRepoNameRegex = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/m; | |||
There was a problem hiding this comment.
Do you have any source which described what repo names are considered as valid by GitHub? Are dots allowed? Why m modifier has been used?
There was a problem hiding this comment.
The Regex was taken from this answer on SO
And yes a little oversight on my end, the m flag is redundant, will remove it, thanks for catching that 👍
| @@ -2,6 +2,8 @@ | |||
| import fetch from "node-fetch"; | |||
| import DataLoader from "dataloader"; | |||
|
|
|||
| const validRepoNameRegex = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/m; | |||
There was a problem hiding this comment.
This should be the equivalent
| const validRepoNameRegex = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/m; | |
| const validRepoNameRegex = /^[\w.-]+\/[\w.-]+$/; |
There was a problem hiding this comment.
why don't we support @ ? like '@ant-design/xxx'
| @@ -121,7 +123,7 @@ export async function getInfo( | |||
| throw new Error("Please pass a commit SHA to getInfo"); | |||
| } | |||
|
|
|||
| if (!request.repo) { | |||
| if (!request.repo || !validRepoNameRegex.test(request.repo)) { | |||
There was a problem hiding this comment.
could you move the second check to a separate if and throw an error that would contain validRepoNameRegex.source? That way users should be able to get a little bit more information as to why the validation has failed
- Correct the repo regex - Move the validation to a separate if statement to provide more detailed message
|
@Andarist I've pushed the changes 🙂 |
|
@Andarist What is blocking this? |
My life :P |
Fixes #509