Skip to content
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

chore: Setup tslib & importHelpers to improve bundle size #10435

Merged
merged 3 commits into from
Oct 6, 2022

Conversation

jimblanc
Copy link
Contributor

@jimblanc jimblanc commented Oct 6, 2022

Description of changes

This change enables importHelpers in our shared build script (build.js) and adds tslib as a dependency to all packages that use the script to improve bundle size.

Note on methodology

I collected the data below by looking at the minified WebPack bundle files that each of our modules produce. While this may not reflect all use-cases, it provides a readily available & consistent metric to evaluate the impact of the change. As far as dependency de-duping & flattening for more complex apps, the numbers below should capture the worst-case where each package includes the minimum number of tslib dependencies that it requires without any additional de-duping that the importing app may perform.

Package Artifact Before Change After Change Bundle Δ
aws-amplify aws-amplify.min.js 1294 KiB 1294 KiB 0 KiB
analytics aws-amplify-analytics.min.js 296 KiB 285 KiB -11 KiB
api aws-amplify-api.min.js 146 KiB 145 KiB -1 KiB
api-graphql aws-amplify-api-graphql.min.js 144 KiB 142 KiB -2 KiB
api-rest aws-amplify-api-rest.min.js 43 KiB 41 KiB -2 KiB
auth aws-amplify-auth.min.js 157 KiB 155 KiB -2 KiB
cache aws-amplify-cache.min.js 15.5 KiB 15.7 KiB +0.2 KiB
core aws-amplify-core.min.js 290 KiB 281 KiB -9 KiB
datastore aws-amplify-datastore.min.js 617 KiB 590 KiB -27 KiB
datastore-storage-adapter aws-amplify-datastore-sqlite-adapter-expo.min.js, aws-amplify-datastore-storage-adapter.min.js 47.6 KiB 44.9 KiB -2.7 KiB
geo aws-amplify-geo.min.js 218 KiB 216 KiB -2 KiB
interactions aws-amplify-interactions.min.js 323 KiB 315 KiB -8 KiB
predictions aws-amplify-predictions.min.js 556 KiB 556 KiB 0 KiB
storage aws-amplify-storage.min.js 328 KiB 322 KiB -6 KiB
xr aws-amplify-xr.min.js 14.7 KiB 13.6 KiB -1.1 KiB

Total Δ for entire project -73.6 KiB

Note Soon-to-be deprecated UI packages, as well as the pushnotification package (which is not used for web & does not use the shared build script) were not included in this change.

Future improvements
  • Refactor build.js so that each package can have its own tsconfig.json
    • This will allow us to disable importHelpers for packages where it doesn't provide any benefit such as cache
  • Upgrade packages that depend on older (< 2.0.0) versions of tslib to improve dependency flattening, for example:
    • @aws-crypto/sha256-js
    • @aws-sdk/credential-provider-cognito-identity
    • @aws-sdk/client-cognito-identity

Issue #, if available

NA

Description of how you validated changes

  • Local release build & unit test pass
  • Manually evaluated WebPack'd bundle sizes

Integ test pass on next-major-version/5: https://app.circleci.com/pipelines/github/aws-amplify/amplify-js/15155/workflows/3a454e6b-c4e8-47aa-9190-359b225d4ffd

  • Existing storage failures already present

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Sorry, something went wrong.

@jimblanc jimblanc marked this pull request as ready for review October 6, 2022 16:37
@jimblanc jimblanc requested review from a team as code owners October 6, 2022 16:37
Copy link
Contributor

@manueliglesias manueliglesias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +163 to +167
// Mock the module to ensure that setters are available for spying
jest.mock('@aws-amplify/core', () => ({
...jest.requireActual('@aws-amplify/core'),
}));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to tslib?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from what I can tell this is kind of a known issue with Jest & TypeScript because babel will only include getters for re-exported functions (like browserOrNode in this file). I'm not entirely sure why this was working before though. I double checked the TS configuration and experimented with setting importHelpers through the ts-jest configuration but it didn't seem to affect anything one way or the other.

@jimblanc jimblanc merged commit 6d98927 into next-major-version/5 Oct 6, 2022
@jimblanc jimblanc deleted the importHelpers-improvement branch October 6, 2022 22:06
jimblanc added a commit that referenced this pull request Oct 20, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
<!--
Please make sure to read the Pull Request Guidelines:

https://github.com/aws-amplify/amplify-js/blob/main/CONTRIBUTING.md#pull-requests
-->

#### Description of changes
This change restores `tslib` dependencies that appear to have been
dropped with a recent merge.

#### Issue #, if available
Original PR with testing details:
#10435

#### Description of how you validated changes
See original PR.

#### Checklist
<!-- Remove items that do not apply. For completed items, change [ ] to
[x]. -->

- [x] PR description included
- [x] `yarn test` passes
- [ ] Tests are [changed or
added](https://github.com/aws-amplify/amplify-js/blob/main/CONTRIBUTING.md#steps-towards-contributions)
- [ ] Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants