fix: replace deprecated deep-assign with lodash merge

[dylmye]

Summary

Replace deep-assign with lodash.merge, as per #374. I understand it's better to use actively maintained packages because it's more likely people already have the package installed, and because if there are any security issues there's a better chance they'll be handled. I know 'if it ain't broke, don't fix it' but this also removes the deprecated warning many people get in their install process.

Other changes:

• Added endOfLine option to prettier rc to avoid this:
  

Test Plan

yarn build e2e:android doesn't work on my Windows machine, I'll test on my Mac when I next can. Behaviour should be the same minus some small edge cases.

[krizzu]

Hey @dylmye

We've already had an attempt at changing this dependency, but (as far as I remember) the problem is that different libraries produce different merge results, so we decided to drop idea of changing.

I'd ideally have one version of "deep-merge" used, but it's tricky due to the fact the implementation of mergeItem is defined per-platform (even jest mock has it's own implementation).

If we were to apply that change, I'd release it as a breaking change.

What do you think @tido64 ?

[dylmye]

Glad to hear JS is being JS. It would just seem in my humble opinion that a breaking change with a maintained package would be a safe bet for the future of this package. Or an internal implementation.

[tido64]

@krizzu: Do you remember what exactly is different with deep-assign? Is the order different or something? Another alternative I can think of is to absorb the parts that we need from deep-assign. If it's simply deprecated and not buggy or anything, we can at least avoid having this warning pop up every time people install AsyncStorage.

[krizzu]

@krizzu: Do you remember what exactly is different with deep-assign? Is the order different or something? Another alternative I can think of is to absorb the parts that we need from deep-assign. If it's simply deprecated and not buggy or anything, we can at least avoid having this warning pop up every time people install AsyncStorage.

Just ran few tests to see the differences between deep-assign and lodash.merge and the main issue is that deep-assign ignores null values. Following source object should override its predecessors.

Merging two objects:

   const test1 = {
      prop1: 'hello!',
      prop2: [1, 2, 3, 4, 5],
      prop3: {
        isDeep: true,
      },
    };

    const test2 = {
      prop1: null,
      prop2: [6, 7, 8],
      prop3: {
        addMe: true,
      },
    };

Should gives us final object with prop1 being null. lodash.merge correctly does that , but deep-assign does not. And that's the only different I could find.

What I found also interesting, is array overriding, where items are not merged together, but rather have their items replaces at indexes. So in our example, (both libs) gives the array [6, 7, 8, 4, 5] as merging results.

Said that, I think we're good to use lodash.merge in both Web storage and Jest - but let's point out in the release notes about the change in libraries

[dylmye]

Said that, I think we're good to use lodash.merge in both Web storage and Jest - but let's point out in the release notes about the change in libraries

Is this something that should be included in the PR? Happy to make those changes

[tido64]

@krizzu: Just for my own understanding, merging the following objects:

const test1 = {
  prop: 'hello!',
};

const test2 = {
  prop1: undefined,
};

Should return:

const testMerged = {
  prop: 'hello!',
};

Correct? If null is the only case where it overwrites, then I think we should rename this PR to "fix: null does not overwrite a property on merge" and merge it. It seems to be the expected behaviour to me.

[krizzu]

@krizzu: Just for my own understanding, merging the following objects:

const test1 = {
  prop: 'hello!',
};

const test2 = {
  prop1: undefined,
};

Should return:

const testMerged = {
  prop: 'hello!',
};

Correct?

Yes, exactly.

If null is the only case where it overwrites, then I think we should rename this PR to "fix: null does not overwrite a property on merge" and merge it. It seems to be the expected behaviour to me.

Yeah, null should override whatever is the current value. I'd agree to name this as a fix, but there might be cases where someone relies on current overriding rule, so it'd be a breaking change for that person.

I guess we agree on getting this merged anyway, but I'll inform about this potential breaking change in release notes.

@dylmye can we also have this used in the mock, instead of built-in one?

[dylmye]

Sorry to leave this for so long! Implemented in tests as you requested @krizzu .

Having trouble running e2e tests locally.

CI windows broken:

error APPX0108: The certificate specified has expired. For more information about renewing certificates, see http://go.microsoft.com/fwlink/?LinkID=241478. [D:\a\async-storage\async-storage\node_modules.generated\windows\ReactTestApp\ReactTestApp.vcxproj]

[tido64]

CI windows broken:

error APPX0108: The certificate specified has expired. For more information about renewing certificates, see http://go.microsoft.com/fwlink/?LinkID=241478. [D:\a\async-storage\async-storage\node_modules.generated\windows\ReactTestApp\ReactTestApp.vcxproj]

Sorry about that. I've got a PR to renew the certificate here: #633

[tido64]

The certificate issue should be fixed now. CI should pass if you rebase on latest.

[krizzu]

@dylmye do you need any help here?

[dylmye]

@dylmye do you need any help here?

I'm on holiday for the next few weeks, sorry about the hold up

[tido64]

@dylmye: Do you have time to finish this? Should one of us take over this PR?

@krizzu: It seems that the other suggestion, merge-options, is a much smaller and more up-to-date implementation of merge. Should we consider using it instead? lodash.merge was last updated 2 years ago, so I'm a bit worried it will become a problem down the line.

[krizzu]

@tido64 sounds good.
This lib has config that modifies the merging behavior (config docs). I'd say to enable both options (ignore undefineds and concatenate arrays), so that we have proper merging in place (with null value overriding other values).

[tido64]

Superseded by #657. Let us know if you're seeing issues.

[dylmye]

Sorry to not get back about this @tido64 - glad to see it progressed. Thanks everyone who helped out!