Addressing-7747;-_.merge-=>-Object.assign #11267

pjmattingly · 2022-07-22T08:19:28Z

Addresses: #7747

Replace _.merge with Object.assign. Note they're not equivalent (_.merge does deep merge), so it should be done with care in all places where it's safe to rely on Object.assign instead (majority of cases)

…cs.js; lib/plugins/invoke.js

done; lib/plugins/aws/package/compile/events/cloud-front.js

…nt.js as does deep merges + noted

lib/plugins/aws/package/compile/events/iot.js lib/plugins/aws/package/compile/events/schedule.js lib/plugins/aws/package/compile/events/sns.js lib/plugins/aws/package/compile/events/sqs.js

lib/plugins/aws/package/compile/events/alb/lib/permissions.js lib/plugins/aws/package/compile/events/api-gateway/lib/api-keys.js lib/plugins/aws/package/compile/events/stream.js

lib/plugins/aws/package/compile/events/api-gateway/lib/authorizers.js lib/plugins/aws/package/compile/events/api-gateway/lib/cors.js lib/plugins/aws/package/compile/events/api-gateway/lib/deployment.js lib/plugins/aws/package/compile/events/api-gateway/lib/resources.js

pjmattingly · 2022-07-22T08:25:56Z

some _.merge instance can't be replaced with Object.assgn; Those files are:

/lib/classes/service.js
/lib/plugins/aws/package/compile/events/cloud-front.js
/lib/plugins/aws/package/compile/events/cognito-user-pool.js
/lib/plugins/aws/package/compile/events/alb/lib/permissions.js
/lib/plugins/aws/package/compile/events/api-gateway/lib/usage-plan.js
/lib/plugins/aws/package/compile/events/api-gateway/lib/method/index.js
/lib/plugins/aws/package/lib/merge-custom-provider-resources.js

Some files had no tests, and so were not modifed; Those files are:

/lib/plugins/aws/package/compile/events/api-gateway/lib/method/responses.js
/lib/plugins/aws/package/compile/events/websockets/lib/api.js
/test/utils/api-gateway.js

Some test files use _.merge, so those were skipped as they weren't in customer-facing code:

/test/unit/lib/configuration/variables/sources/instance-dependent/get-aws.test.js
/test/unit/lib/configuration/variables/sources/instance-dependent/get-sls.test.js

codecov · 2022-07-22T08:26:19Z

Codecov Report

Merging #11267 (c799df6) into main (bb37f4f) will decrease coverage by 0.02%.
The diff coverage is 97.10%.

@@            Coverage Diff             @@
##             main   #11267      +/-   ##
==========================================
- Coverage   85.89%   85.87%   -0.03%     
==========================================
  Files         315      315              
  Lines       13247    13228      -19     
==========================================
- Hits        11378    11359      -19     
  Misses       1869     1869

Impacted Files	Coverage Δ
lib/classes/plugin-manager.js	`93.72% <ø> (ø)`
.../plugins/aws/package/compile/events/cloud-front.js	`97.70% <ø> (ø)`
...aws/package/lib/merge-custom-provider-resources.js	`78.78% <ø> (ø)`
lib/classes/service.js	`84.61% <66.66%> (ø)`
...package/compile/events/api-gateway/lib/rest-api.js	`91.42% <83.33%> (ø)`
lib/classes/config.js	`100.00% <100.00%> (ø)`
lib/classes/console.js	`81.25% <100.00%> (ø)`
lib/cli/commands-schema/resolve-final.js	`90.74% <100.00%> (ø)`
lib/plugins/aws/invoke-local/index.js	`69.87% <100.00%> (ø)`
lib/plugins/aws/metrics.js	`67.24% <100.00%> (ø)`
... and 31 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 bb37f4f...c799df6. Read the comment docs.

pgrzesik

Thanks a lot @pjmattingly - it looks good in general! I have a few comments, please let me know what do you think 🙇

pgrzesik · 2022-07-22T11:04:58Z

lib/classes/config.js

@@ -12,7 +11,7 @@ class Config {
  }

  update(config) {
-    return _.merge(this, config);
+    return Object.assign(this, config);


Did you validate that it's going to be a safe change? It seems like a pretty generic method where deep vs shallow copy behavior might have an impact

pgrzesik · 2022-07-22T11:06:32Z

lib/classes/service.js

@@ -284,7 +284,7 @@ class Service {
  }

  update(data) {
-    return _.merge(this, data);
+    return Object.assign(this, data);


Is it safe to use Object.assign here? Is the deep merge not needed here?

pgrzesik · 2022-07-22T11:06:44Z

lib/classes/service.js

@@ -346,7 +346,7 @@ class Service {

  publish(dataParam) {
    const data = dataParam || {};
-    this.pluginsData = _.merge(this.pluginsData, data);
+    this.pluginsData = Object.assign(this.pluginsData, data);


Is it safe to use Object.assign here? Is the deep merge not needed here?

pgrzesik · 2022-07-22T11:10:38Z

lib/plugins/aws/package/compile/events/cloud-front.js

@@ -612,6 +612,7 @@ class AwsCompileCloudFrontEvents {

      Object.assign(Resources, { [this.cloudFrontDistributionLogicalId]: CloudFrontDistribution });

+      // #7747, could not replace _.merge with Object.assign, as this does deep merges


I think these assignments look pretty safe to be refactored - do you think we should keep merge here?

medikoo

@pgrzesik @pjmattingly with this change (while it's highly desirable) we need to be very careful.

If we touch not well tested areas it's possible we may introduce regressions, as behavior of _.merge and Object.assign differs.

Ideally, if each change is validated individually by looking through carefully what it does and what further logic it affects.

I would also add a comment in each case where we leave _.merge usage with reasoning, on why _.merge is wanted approach

@pjmattingly please confirm also by running integration tests, that we did not break anything

pjmattingly · 2022-07-25T20:57:28Z

@medikoo

please confirm also by running integration tests, that we did not break anything

Will do.

I would also add a comment in each case where we leave _.merge usage with reasoning, on why _.merge is wanted approach

I'll find my notes and add some more detail in comments for the unchanged ones.

Ideally, if each change is validated individually by looking through carefully what it does and what further logic it affects.

I was working on #11273 and I found a need for inspecting objects during testing (See also: https://www.reddit.com/r/javascript/comments/w7ial4/askjs_a_library_for_tracking_the_properties_of_an/?). I think such functionality might also be helpful here, as the properties for the object can be tracked during (integration) testing. Would you agree?

If so, then it might be worth making / finding code to track and report on the different values that effected objects go through and potentially where the code is called from. This way we can have more confidence about: 1) What properties and values the objects typically take and 2) What external code these changes touch.

What do you think?

medikoo · 2022-07-26T07:43:35Z

If so, then it might be worth making / finding code to track and report on the different values that effected objects go through and potentially where the code is called from.

I'm not sure if it's necessary to bundle such a thing into a test suite. It feels a bit overkill (and definitely adds to technical debt), but if in this specific case, it would help you test whether change is not breaking. Feel free to set up such thing temporarily (just to test proposed changes, but not be a part of a PR)

pjmattingly · 2022-07-26T08:28:21Z

Exactly. Thanks for weighing in!

…

On Tue, Jul 26, 2022 at 1:43 AM Mariusz Nowak ***@***.***> wrote: If so, then it might be worth making / finding code to track and report on the different values that effected objects go through and potentially where the code is called from. I'm not sure if it's necessary to bundle such a thing into a test suite. It feels a bit overkill (and definitely adds to technical debt), but if in this specific case, it would help you test whether change is not breaking. Feel free to set up such thing temporarily (just to test proposed changes, but not be a part of a PR) — Reply to this email directly, view it on GitHub <#11267 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABP7MZ7HYF4H7TIH24HDBK3VV6JLFANCNFSM54KPBF2Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pjmattingly · 2022-07-28T13:38:13Z

Integration tests look good.

Next, adding notes about why leaving in some _.merge()s. Then, getting a closer look at each change to determine the inputs and outputs, and what other code is effected by these changes indirectly.

I would also add a comment in each case where we leave _.merge usage with reasoning, on why _.merge is wanted approach

Ideally, if each change is validated individually by looking through carefully what it does and what further logic it affects.

medikoo · 2022-07-28T15:44:11Z

@pjmattingly once it's ready for re-review, it's best if you simply re-request review in right top box

pjmattingly · 2022-07-29T04:30:27Z

Okay, will do. I as making notes to show progress toward completion; It helps me remember what I was doing with this PR.

Is that okay?

medikoo · 2022-07-29T08:24:35Z

Okay, will do. I as making notes to show progress toward completion; It helps me remember what I was doing with this PR.
Is that okay?

Yes, that's totally good. Great thanks for helping on that one 👍

pjmattingly added 21 commits July 21, 2022 23:37

addressing-7747;-config.js

e690267

done, lib/classes

0977163

done, lib/classes/plugin-manager.js

ac2f15e

done, lib/classses/service.js

3ec9b35

done, lib/cli/commands-schema/resolve-final.js; lib/plugins/aws/metri…

c1f5adb

…cs.js; lib/plugins/invoke.js

done; lib/plugins/aws/invoke-local/index.js

15cd643

done; lib/plugins/aws/package/compile/layers.js

66f2e1b

done; lib/plugins/aws/package/compile/events/alexa-skill.js

92389b4

done; lib/plugins/aws/package/compile/events/alexa-smart-home.js

b491cf5

done; lib/plugins/aws/package/compile/events/cloud-front.js

reverting changes to lib/plugins/aws/package/compile/events/cloud-fro…

0ba5104

…nt.js as does deep merges + noted

done; lib/plugins/aws/package/compile/events/cloud-watch-event.js

76bf222

done; lib/plugins/aws/package/compile/events/cloud-watch-log.js

ec6997c

done; lib/plugins/aws/package/compile/events/cognito-user-pool.js

3fafa71

done

b08e454

lib/plugins/aws/package/compile/events/iot.js lib/plugins/aws/package/compile/events/schedule.js lib/plugins/aws/package/compile/events/sns.js lib/plugins/aws/package/compile/events/sqs.js

done

482551e

lib/plugins/aws/package/compile/events/alb/lib/permissions.js lib/plugins/aws/package/compile/events/api-gateway/lib/api-keys.js lib/plugins/aws/package/compile/events/stream.js

done

481c5a8

lib/plugins/aws/package/compile/events/api-gateway/lib/authorizers.js lib/plugins/aws/package/compile/events/api-gateway/lib/cors.js lib/plugins/aws/package/compile/events/api-gateway/lib/deployment.js lib/plugins/aws/package/compile/events/api-gateway/lib/resources.js

done

f985926

done

d9ffced

done

801b362

done

69a2af4

done

c799df6

pgrzesik added the refactor label Jul 22, 2022

pgrzesik suggested changes Jul 22, 2022

View reviewed changes

medikoo reviewed Jul 25, 2022

View reviewed changes

pjmattingly mentioned this pull request Jul 26, 2022

TypeError: CloudFormation is not a constructor function #11281

Closed

4 tasks

pjmattingly mentioned this pull request Jul 28, 2022

test/integration/curated-plugins-python.test.js failed #11297

Open

4 tasks

pjmattingly closed this by deleting the head repository Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing-7747;-_.merge-=>-Object.assign #11267

Addressing-7747;-_.merge-=>-Object.assign #11267

pjmattingly commented Jul 22, 2022 •

edited

pjmattingly commented Jul 22, 2022 •

edited

codecov bot commented Jul 22, 2022

pgrzesik left a comment

pgrzesik Jul 22, 2022

pgrzesik Jul 22, 2022

pgrzesik Jul 22, 2022

pgrzesik Jul 22, 2022

medikoo left a comment

pjmattingly commented Jul 25, 2022

medikoo commented Jul 26, 2022

pjmattingly commented Jul 26, 2022 via email

pjmattingly commented Jul 28, 2022

medikoo commented Jul 28, 2022

pjmattingly commented Jul 29, 2022

medikoo commented Jul 29, 2022

		@@ -612,6 +612,7 @@ class AwsCompileCloudFrontEvents {

		Object.assign(Resources, { [this.cloudFrontDistributionLogicalId]: CloudFrontDistribution });

		// #7747, could not replace _.merge with Object.assign, as this does deep merges

Addressing-7747;-_.merge-=>-Object.assign #11267

Addressing-7747;-_.merge-=>-Object.assign #11267

Conversation

pjmattingly commented Jul 22, 2022 • edited

pjmattingly commented Jul 22, 2022 • edited

codecov bot commented Jul 22, 2022

Codecov Report

pgrzesik left a comment

Choose a reason for hiding this comment

pgrzesik Jul 22, 2022

Choose a reason for hiding this comment

pgrzesik Jul 22, 2022

Choose a reason for hiding this comment

pgrzesik Jul 22, 2022

Choose a reason for hiding this comment

pgrzesik Jul 22, 2022

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

pjmattingly commented Jul 25, 2022

medikoo commented Jul 26, 2022

pjmattingly commented Jul 26, 2022 via email

pjmattingly commented Jul 28, 2022

medikoo commented Jul 28, 2022

pjmattingly commented Jul 29, 2022

medikoo commented Jul 29, 2022

pjmattingly commented Jul 22, 2022 •

edited

pjmattingly commented Jul 22, 2022 •

edited