-
Notifications
You must be signed in to change notification settings - Fork 987
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 |
schmidt-sebastian
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 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.
| } | ||
| } | ||
|
|
||
| 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
connectionURLand its helper methodneedsQueryParambut let me know if that's incorrect.