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

Replace lodash forEach(array) with native Array.forEach #7748

Merged
merged 6 commits into from May 28, 2020

Conversation

exoego
Copy link
Contributor

@exoego exoego commented May 19, 2020

Adresses #7747

All occurence of _.forEach(array, e => ... are replaced with native array.forEach(e => ...).

_.forEach(object, (k, v) => ....) still remains, since there are no native counterpart.
(Maybe Object.entries(obj).forEach(pair => { const [k,v] = pair; ..., which is verbose)
_.each (alias of _.forEach) are renamed to _.forEach.

One caveat of native array.forEach is null pointer error.
If the value is null, array.forEach throws NPE.
However, _.forEach(null, ...) ignores null silently.
So we might need (value || []).forEach or similar to avoid NPE.

@exoego exoego force-pushed the foreach branch 2 times, most recently from 6285f3f to 486ace5 Compare May 20, 2020 00:15
@exoego exoego changed the title Replace lodash forEach(array) with natie Array.forEach Replace lodash forEach(array) with native Array.forEach May 20, 2020
@exoego exoego closed this May 20, 2020
@exoego exoego reopened this May 20, 2020
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@exoego great thanks for that!

Unfortunately tests failed, so something got broken.

Concerning objects, I believe we need to refactor it to Object.entries, but we cannot do that now, we need to wait for #7362

@exoego exoego force-pushed the foreach branch 2 times, most recently from e4a6416 to 9511532 Compare May 20, 2020 13:28
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #7748 into master will increase coverage by 0.00%.
The diff coverage is 94.44%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7748   +/-   ##
=======================================
  Coverage   88.13%   88.14%           
=======================================
  Files         245      245           
  Lines        9262     9261    -1     
=======================================
  Hits         8163     8163           
+ Misses       1099     1098    -1     
Impacted Files Coverage Δ
lib/utils/log/log.js 0.00% <0.00%> (ø)
lib/plugins/aws/info/getApiKeyValues.js 92.30% <50.00%> (ø)
lib/classes/CLI.js 95.20% <100.00%> (ø)
lib/classes/PluginManager.js 96.67% <100.00%> (ø)
lib/classes/Service.js 97.47% <100.00%> (ø)
lib/plugins/aws/deploy/lib/extendedValidate.js 97.56% <100.00%> (ø)
lib/plugins/aws/info/display.js 82.14% <100.00%> (ø)
...ins/aws/package/compile/events/alb/lib/validate.js 96.66% <100.00%> (ø)
...s/package/compile/events/apiGateway/lib/apiKeys.js 94.59% <100.00%> (ø)
...ompile/events/apiGateway/lib/method/integration.js 98.21% <100.00%> (ø)
... and 12 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 d43241e...57bc5e7. Read the comment docs.

@exoego
Copy link
Contributor Author

exoego commented May 26, 2020

@medikoo Ready to re-review.

@exoego exoego mentioned this pull request May 26, 2020
54 tasks
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@exoego great thanks for update! Still, it appears it now has conflicts, can you fix them?

# Conflicts:
#	lib/plugins/aws/package/compile/events/cognitoUserPool/index.js
# Conflicts:
#	lib/plugins/aws/package/compile/events/apiGateway/lib/apiKeys.test.js
#	lib/plugins/aws/package/compile/events/apiGateway/lib/usagePlanKeys.test.js
#	lib/plugins/aws/package/compile/functions/index.js
#	lib/plugins/package/lib/zipService.js
@exoego
Copy link
Contributor Author

exoego commented May 28, 2020

@medikoo Sorry for delay.
Conflict was resolved.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @exoego !

@medikoo medikoo merged commit 5e0af21 into serverless:master May 28, 2020
@exoego exoego deleted the foreach branch January 10, 2022 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants