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 utility with String native methods #7715

Merged
merged 1 commit into from
May 19, 2020

Conversation

exoego
Copy link
Contributor

@exoego exoego commented May 11, 2020

See #7680 (comment)

Let's minimize lodash dependency where native ways are available.

  • Better performance thanks to native implementation
  • Smaller bundle size thanks to depend only small portion of lodash (e.g. require('lodash.pick'))

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.

Great thanks @exoego, a worthwhile cleanup.

Still it appears negation was lost in two places (CI picked that as well)

@@ -13,7 +12,7 @@ module.exports = {
path.join(this.serverless.config.servicePath || '.', '.serverless');

// Only move the artifacts if it was requested by the user
if (this.serverless.config.servicePath && !_.endsWith(packagePath, '.serverless')) {
if (this.serverless.config.servicePath && packagePath.endsWith('.serverless')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I see correctly negation was lost

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and squashed commits.

@@ -36,7 +35,7 @@ module.exports = {
path.join(this.serverless.config.servicePath || '.', '.serverless');

// Only move the artifacts if it was requested by the user
if (this.serverless.config.servicePath && !_.endsWith(packagePath, '.serverless')) {
if (this.serverless.config.servicePath && packagePath.endsWith('.serverless')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and squashed commits.

@codecov-io
Copy link

Codecov Report

Merging #7715 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7715      +/-   ##
==========================================
- Coverage   88.00%   87.99%   -0.02%     
==========================================
  Files         245      245              
  Lines        9202     9201       -1     
==========================================
- Hits         8098     8096       -2     
- Misses       1104     1105       +1     
Impacted Files Coverage Δ
lib/plugins/package/lib/zipService.js 94.64% <ø> (ø)
lib/classes/PluginManager.js 96.67% <100.00%> (ø)
lib/plugins/aws/common/lib/artifacts.js 100.00% <100.00%> (ø)
lib/plugins/aws/deploy/lib/checkForChanges.js 98.19% <100.00%> (ø)
.../aws/package/compile/events/apiGateway/lib/cors.js 100.00% <100.00%> (ø)
...s/package/compile/events/apiGateway/lib/restApi.js 98.00% <100.00%> (ø)
lib/plugins/aws/package/compile/functions/index.js 96.73% <100.00%> (-0.02%) ⬇️
lib/plugins/aws/lib/naming.js 98.07% <0.00%> (ø)
lib/utils/standalone-patch.js 0.00% <0.00%> (ø)

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 dd9bf9a...a2f3e98. Read the comment docs.

@medikoo
Copy link
Contributor

medikoo commented May 19, 2020

@exoego is this ready to take? If so, please re-request review. Thank you!

@exoego
Copy link
Contributor Author

exoego commented May 19, 2020

Ready to re-review.

@exoego exoego requested a review from medikoo May 19, 2020 08:57
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 8bb5517 into serverless:master May 19, 2020
@exoego exoego deleted the string-native-methods 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.

3 participants