Add bundle definitions and its measurement script. #5706
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Binary Size ReportAffected SDKs
Test Logs |
Size Analysis ReportAffected Products
|
hsubox76
reviewed
Nov 8, 2021
Changeset File Check ✅
|
yifanyang
force-pushed
the
yifany/bundle-size
branch
from
0069054 to
a9c4167
Compare
yifanyang
force-pushed
the
yifany/bundle-size
branch
from
99af8dd to
1a12d6e
Compare
hsubox76
reviewed
Nov 9, 2021
| { | ||
| "path": "app-check", | ||
| "imports": [ | ||
| "initializeAppCheck", |
There was a problem hiding this comment.
Did these used to include getToken? I thought I remembered seeing them. It makes sense because if app check is being used, getToken() will eventually be called, if not by the user then by another library (storage, functions, etc).
There was a problem hiding this comment.
Yes, it was there before. I got confused this morning when reading the code sample for App Check Enterprise, which doesn't reference getToken, and I wondered why it was there. I forgot that it was actually Feiyang who added getToken for me in the bundle doc last week. I was actually about to ask you, but got stuck in some changeset warnings in this pull request.
Anyway, I have added it back both in the bundle definition doc and the json files in this pull request. Please take a look. Thanks!
egilmorez
reviewed
Nov 9, 2021
hsubox76
approved these changes
Nov 10, 2021
Feiyang1
reviewed
Nov 10, 2021
| const analyses = JSON.parse(fs.readFileSync(output, { encoding: 'utf-8' })); | ||
| const results: Report[] = []; | ||
| for (const analysis of analyses) { | ||
| const sdk = 'bundle'; |
There was a problem hiding this comment.
The name we put here will be appear as the name of a new section under "affected sdks" in a PR report. For example, there is another section named "bundle" in the report above in this PR [1]. We can put whatever name we want here.
[1] #5706 (comment)
I suppose it is a limitation of the metric backend at the moment. When the API for receiving size measurements was first designed, the backend only recognizes http requests with a json body strictly in this format:
[
{
sdk: <some-string>,
type: <some-string>,
value: <some-integer>
},
......
]
Writing a new API in the backend and deploying a new version of the service to GCP can be some work, therefore I'm reusing the API for bundle measurements here, although the semantic of the API doesn't make any sense in the context of "bundle-analysis". What do you think?
There was a problem hiding this comment.
I think it's fine. It would be great if you can copy your comment here to the code.
There was a problem hiding this comment.
Added some comment in the code to explain the rationale here.
| @@ -0,0 +1,202 @@ | |||
| [ | |||
There was a problem hiding this comment.
@sam-gc Can you please take a look at the Auth use cases to see if you want to add anything, or if anything is inaccurate?
For context, each object in the array is a bundle definition for a specific use case. It's basically a list of imports that customers need to import to use a certain feature from our products.
We are setting up a dashboard to continuously track the bundle size for them across versions.
| @@ -0,0 +1,256 @@ | |||
| [ | |||
| { | |||
| "name": "Read data once", | |||
There was a problem hiding this comment.
@schmidt-sebastian Can you please take a look at the Database/Firestore/Storage use cases to see if you want to add anything, or if anything is inaccurate?
For context, each object in the array is a bundle definition for a specific use case. It's basically a list of imports that customers need to import to use a certain feature from our products.
We are setting up a dashboard to continuously track the bundle size for them across versions.
Feiyang1
reviewed
Nov 10, 2021
|
Thanks for setting up the bundle size tracking! It looks good. It would be great if you can add some comments in |
I have added some jsdoc on the method. Please take a look. |
Feiyang1
reviewed
Nov 11, 2021
Comment on lines
41
to
42
| * @param {string} [version] - If present, the SDK version to run measurement against | ||
| * @returns {Promise<Report[]>} A list of bundle size measurements |
There was a problem hiding this comment.
We use tsdoc instead of jsdoc. Please update. You can take a look at any of our packages for reference.
There was a problem hiding this comment.
Is that just a standard of doc format or something I should use to generate the doc?
There was a problem hiding this comment.
Done. Changed to tsdoc format.
2 tasks
Feiyang1
approved these changes
Nov 12, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.