-
Notifications
You must be signed in to change notification settings - Fork 938
Tree-shake RepoInfo #4515
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
Tree-shake RepoInfo #4515
Conversation
Binary Size ReportAffected SDKs
Test Logs
|
Size Analysis Report |
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 it is pretty hard to determine which methods actually make a difference in terms of bundle size until we have the API implemented. I think your change makes sense. If there is anything that stands out once we have a tree-shakeable API we can always change it.
@@ -133,3 +91,51 @@ export class RepoInfo { | |||
return `${protocol}${this.host}/${query}`; | |||
} | |||
} | |||
|
|||
function needsQueryParam(repoInfo: RepoInfo): boolean { |
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.
Nit: I would also prefix this with "repoInfo" for consistency.
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.
It seems like most of the methods should stay on the class, I only extracted
connectionURL
and its helper methodneedsQueryParam
but let me know if that's incorrect.