Skip to content

Adds lodash as an explicit dependency. #6131

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.

Sign up for GitHub

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

Jump to bottom
Merged
amhinson merged 4 commits into from
Aug 20, 2020
Merged

Adds lodash as an explicit dependency. #6131

amhinson merged 4 commits into from
Aug 20, 2020

Conversation

@quisido
Copy link
Contributor

@quisido quisido commented Jun 21, 2020

Using Yarn2, I get the following (valid) error:

Module not found: @aws-amplify/analytics tried to access lodash, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

You can see imports from lodash in both of these files:

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

@quisido
Adds lodash as an explicit dependency.
9ad6e04 
Using Yarn2, I get the following (valid) error:

```
Module not found: @aws-amplify/analytics tried to access lodash, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.
```

You can see imports from `lodash` in both of these files:
* https://github.com/aws-amplify/amplify-js/blob/e85640a4e18fa1d55411038fee58919ad73121fa/packages/analytics/src/Providers/AmazonPersonalizeHelper/SessionInfoManager.ts
* https://github.com/aws-amplify/amplify-js/blob/e85640a4e18fa1d55411038fee58919ad73121fa/packages/analytics/src/Providers/AmazonPersonalizeProvider.ts
@quisido quisido requested a review from iartemiev as a code owner June 21, 2020 02:06
@codecov
Copy link

codecov bot commented Jun 21, 2020 •
edited
Loading

Codecov Report

Merging #6131 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6131   +/-   ##
=======================================
  Coverage   73.29%   73.30%           
=======================================
  Files         208      208           
  Lines       12925    12928    +3     
  Branches     2433     2526   +93     
=======================================
+ Hits         9474     9477    +3     
+ Misses       3288     3260   -28     
- Partials      163      191   +28
Impacted Files Coverage Δ
...ders/AmazonPersonalizeHelper/SessionInfoManager.ts 89.70% <100.00%> (+0.15%) ⬆️
...alytics/src/Providers/AmazonPersonalizeProvider.ts 74.56% <100.00%> (+0.29%) ⬆️
packages/auth/src/OAuth/OAuth.ts 56.11% <0.00%> (ø)
packages/core/src/Credentials.ts 31.67% <0.00%> (ø)
packages/analytics/src/Analytics.ts 64.81% <0.00%> (ø)
packages/datastore/src/sync/index.ts 15.38% <0.00%> (ø)
packages/datastore/src/sync/outbox.ts 24.48% <0.00%> (ø)
packages/datastore/src/storage/storage.ts 71.66% <0.00%> (ø)
packages/core/src/OAuthHelper/GoogleOAuth.ts 33.33% <0.00%> (ø)
packages/core/src/Util/Reachability.native.ts 37.50% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9056785...55a382a. Read the comment docs.

@amhinson
Copy link
Contributor

amhinson commented Aug 20, 2020

@CharlesStover thanks for the PR! After talking with @iartemiev offline, what do you think about adding these 3 functions that are being used directly as dependencies instead of the entire library?

For example:

amplify-js/packages/aws-amplify-vue/package.json

Line 16 in d6035dd

"lodash.orderby": "^4.6.0",

amplify-js/packages/aws-amplify-vue/src/components/authenticator/SignUp.vue

Line 63 in e85640a

import orderBy from 'lodash.orderby';

@quisido
Copy link
Contributor Author

quisido commented Aug 20, 2020

I think minimal implementations of lodash are preferable, but the two files I referenced in the OP import from the lodash library as a whole, so the entire lodash library needs to be installed in order for those imports to succeed.
I no longer have this package pulled to do a project-wide search for lodash imports and refactor as necessary.

@amhinson amhinson requested a review from sammartinez August 20, 2020 19:50
sammartinez
sammartinez reviewed Aug 20, 2020
View reviewed changes
packages/analytics/src/Providers/AmazonPersonalizeHelper/SessionInfoManager.ts
import isEmpty from 'lodash/isEmpty';
import isEqual from 'lodash/isEqual';
import { v1 as uuid } from 'uuid';
import { StorageHelper, ConsoleLogger as Logger, JS } from '@aws-amplify/core';
Copy link
Contributor

@sammartinez sammartinez Aug 20, 2020

Choose a reason for hiding this comment

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

Do we want to remove the StorageHelper here?

Copy link
Contributor

@amhinson amhinson Aug 20, 2020

Choose a reason for hiding this comment

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

It wasn't being used (along with RequestParams) so I just removed them.

sammartinez
sammartinez approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

@lgtm-com
Copy link

lgtm-com bot commented Aug 20, 2020

This pull request fixes 2 alerts when merging 55a382a into 9056785 - view on LGTM.com

fixed alerts:

  • 2 for Unused variable, import, function or class

amhinson
amhinson approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@amhinson amhinson left a comment

Choose a reason for hiding this comment

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

Thanks again @CharlesStover! 👍

@amhinson amhinson merged commit 00d5e17 into aws-amplify:main Aug 20, 2020
@ericclemmons ericclemmons mentioned this pull request Sep 17, 2020
Merged
nubpro pushed a commit to nubpro/amplify-js that referenced this pull request Oct 2, 2020
@quisido @amhinson @nubpro
Adds lodash as an explicit dependency. (aws-amplify#6131)
9e95c8f 
* Adds lodash as an explicit dependency.
* update lodash imports

Co-authored-by: Alex Hinson <[email protected]>
iartemiev pushed a commit that referenced this pull request Oct 13, 2020
@quisido @amhinson @iartemiev
Adds lodash as an explicit dependency. (#6131)
01a6c82 
* Adds lodash as an explicit dependency.
* update lodash imports

Co-authored-by: Alex Hinson <[email protected]>
@github-actions
Copy link

github-actions bot commented Aug 20, 2021

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@iartemiev iartemiev Awaiting requested review from iartemiev

2 more reviewers

@sammartinez sammartinez sammartinez approved these changes

@amhinson amhinson amhinson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@quisido @amhinson @sammartinez @iartemiev