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: Move cache sideEffects to align with other packages #10562

Merged

Conversation

jimblanc
Copy link
Contributor

@jimblanc jimblanc commented Oct 28, 2022

Description of changes

This change moves the side effects in cache out of index.ts (and reactnative.ts) to align with the other categories. It also removes cache as a default static member (currently unused) of the Amplify singleton in order to reduce bundle size for use cases that don't directly use cache. Finally, it removes cache as a dependency of auth.

Observed bundle size improvements in a barebones CRA app:
Parsed: -15.1 kB
GZipped: -2.06 kB

Issue #, if available

Description of how you validated changes

Local testing.

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.

@jimblanc jimblanc marked this pull request as ready for review October 28, 2022 18:40
@jimblanc jimblanc requested a review from a team as a code owner October 28, 2022 18:40
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022

Codecov Report

Merging #10562 (d9d93db) into next-major-version/5 (46c8d34) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@                   Coverage Diff                    @@
##           next-major-version/5   #10562      +/-   ##
========================================================
+ Coverage                 84.65%   84.67%   +0.01%     
========================================================
  Files                       195      195              
  Lines                     17439    17438       -1     
  Branches                   3693     3693              
========================================================
+ Hits                      14763    14765       +2     
+ Misses                     2597     2594       -3     
  Partials                     79       79              
Impacted Files Coverage Δ
packages/aws-amplify/src/amplifySingleton.ts 100.00% <ø> (ø)
packages/cache/src/BrowserStorageCache.ts 93.81% <100.00%> (+0.03%) ⬆️
...kages/amazon-cognito-identity-js/src/BigInteger.js 89.71% <0.00%> (+0.40%) ⬆️
packages/cache/src/StorageCache.ts 81.13% <0.00%> (+1.88%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jimblanc jimblanc marked this pull request as draft October 28, 2022 18:56
@jimblanc jimblanc marked this pull request as ready for review October 28, 2022 19:15
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Looks good!

Thanks @jimblanc 🔥


/** Always importing Auth when users import Amplify such that
for unauthenticated access (no sign in and sign up),
users don't have to import Auth explicitly **/
Amplify.Auth = Auth;
Amplify.Cache = Cache;
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 technically a breaking change if someone were accessing the Cache via the singleton?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just making sure we call out this breaking change in v5 changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep it would be, it'll have to go into the changelog (this usage pattern has been deprecated for a while I believe).

Comment on lines +4 to +5
import { Amplify, ConsoleLogger as Logger } from '@aws-amplify/core';
import AsyncStorage from '@react-native-async-storage/async-storage';
Copy link
Contributor

Choose a reason for hiding this comment

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

@jimblanc jimblanc merged commit ed196f5 into aws-amplify:next-major-version/5 Oct 28, 2022
@jimblanc jimblanc deleted the chore/cache-sideEffects branch October 28, 2022 20:33
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.

None yet

4 participants