Remove extend dependency, becoming a zero-dependency package

[jonathantneal]

This PR replaces the lone extend dependency with an explicit function, possibly improving the readability ^[a] of index.js.

^a: this package only extends plain objects, and it extends them in a unique way, where any two nested objects are merged into one new object, rather than continually merging into the former. This unique strategy seems useful to have in the open.

[foolip]

I can confirm that this produces exactly the same object (compared after JSON.stringify) as before the change.

It would be great if the new extend could be tested, but to do that would require breaking it out into a helper that's not excluded by .npmignore, so I'll leave it to @ddbeck to say if that's worthwhile.

[foolip]

Some ideas, but ultimately these are suggestions, what's in this PR seems to already work just fine.

[foolip]

Including inherited properties should be harmless since there aren't any given that a and b were parsed from JSON, but it should also be unnecessary. Could Object.keys() be used here, to end up with behavior more like Object.assign() which copies enumerable own properties?

[jonathantneal]

Thank you for all of these thoughtful comments and questions. I also agree so much!

This leniency was done purposefully, because that’s how it behaved in the old (jQuery) extend method. From the extend package documentation:

... properties inherited from the object's prototype will be copied over.

This is why I added the explicit comment. At least now it’s out in the open? 😅

[foolip]

Yes, very good to shine a light on it. It made me nervous since in BCD we have properties like "toString" and "hasOwnProperty" where p in {} would return true. It seems like we don't have any accidents because of it, but dealing only with enumerable own properties seems preferable, or really matching whatever the behavior of Object.assign is. (I'm still not sure about the case where there's an enumerable own property on the one side and a non-enumerable own property on the other. We can't have that in BCD, though.)

[foolip]

This merging logic is more lenient than BCD needs, and would allow for weird errors in the BCD data which would go unnoticed unless there's a lint for it. This might be a good opportunity to simplify this to make more assumptions about the data, throwing exceptions if they're not met.

I think that any time a property exists on both a and b, both values must be regular objects (not arrays, for example), otherwise something is wrong.

[foolip]

Doing this will reveal that there is some broken structure in BCD, the first one I run into is "bcd.css.selectors.not_match_link" defined in multiple files by accident.

[foolip]

I've fixed that in #9842.

[jonathantneal]

This might be a good opportunity to simplify this to make more assumptions about the data, throwing exceptions if they're not met.

Yes. I’d like to help here, if I can. 😄

This PR aims to provide a simplified method that would not throw new errors, and then also honor the ** peculiar** bits from the (jQuery) extend dependency implementation; versus something like a deep Object.assign.

I see two options, and defer to your preference. One option is to "do the most", and improve the usefulness of this function and then bundle any resulting fixes that are surfaced. The other option is to "do the least", and approve or improve this function to replace the existing dependency, with its peculiar bits and leniencies out in the open.

[foolip]

With that fixed, here's a less forgiving variant:

function isPlainObject(v) {
  return typeof v === 'object' && v !== null && !Array.isArray(v);
}

function extend(target, source) {
  if (!isPlainObject(target) || !isPlainObject(source)) {
    throw new Error('Both target and source must be plain objects');
  }
  // iterate over all enumerable properties
  for (const [key, value] of Object.entries(source)) {
    // recursively extend if target has the same key, otherwise just assign
    if (Object.prototype.hasOwnProperty.call(target, key)) {
      extend(target[key], value);
    } else {
      target[key] = value;
    }
  }

  return target;
}

Feel free to use as much or as little of that as you like.

[foolip]

Sorry, I commented before I read your comment. A deep Object.assign is indeed what I think we should use here, something that throws if used with arrays in particular. Object.assign({}, null) doesn't throw, but that's a case we should also disallow to catch more errors I think.

[jonathantneal]

Hi, @foolip! After rebasing with the latest main branch and updating the package-lock.json file, everything continues to run as expected.

However, if I update the extend function with the less forgiving variant then the lint command fails.

The error is rather catastrophic on a terminal as the linter attempts to log massive objects as being invalid, but it starts like this:

✖ api/ANGLE_instanced_arrays.json
  JSON Schema – 1 error:

I suspect the deep mutation of target causes this error, and something else expects deep objects not to change.

I also tried updating the function to always return a new plain object, and the lint command failed again. This time, I suspect the missing shallow mutation of target causes this error! Haha.

This place in the middle works, and is done in bde56ca 🎉

function isPlainObject(v) {
  return typeof v === 'object' && v !== null && !Array.isArray(v);
}

function extend(target, source) {
  if (!isPlainObject(target) || !isPlainObject(source)) {
    throw new Error('Both target and source must be plain objects');
  }

  // iterate over all enumerable properties
  for (const [key, value] of Object.entries(source)) {
    // recursively extend if target has the same key, otherwise just assign
    if (Object.prototype.hasOwnProperty.call(target, key)) {
      target[key] = extend(extend({}, target[key]), value);
    } else {
      target[key] = value;
    }
  }

  return target;
}

Of course, I’m glad to continue working with you on this, if you’d like to remove this last peculiar bit. 😄

[foolip]

Are two extend calls needed here? Since extend modifies a in-place, I think this could be just extend(oldValue, newValue), assuming the a[name] = newValue is done in an else branch and not unconditionallly.

[jonathantneal]

Yes, I agree that this is peculiar, which is a reason I think it belongs in the codebase and not abstracted away.

From my PR, with emphasis added:

... it extends them in a unique way, where any two nested objects are merged into one new object

The (jQuery) extend method mutates a root object, but it does not mutate any of its nested objects.

If this (new) extend method were simplified so that the nested objects were also mutated, then many tests would fail.

From those failures I inferred that this particular functionality is by design.

[foolip]

I defer to @ddbeck, but I'd say that as long as the resulting object is identical to before, various peculiarities of extend that we don't depend on need not be preserved, indeed it's better if they don't work :)

[ddbeck]

I defer to @ddbeck, but I'd say that as long as the resulting object is identical to before, various peculiarities of extend that we don't depend on need not be preserved, indeed it's better if they don't work :)

I agree with this. I'm fine with making this less weird, if the resulting object is the same or it breaks in ways that reveal genuine data problems (as in #9842).

I think extend was originally adopted when BCD was quite immature and it was convenient for experimentation, particularly when the schema went through some major changes early on (before my time—@Elchi3 can speak to this history better than I can). I don't think we've ever actually benefited from any of extend's oddities.

So I guess what I'm saying here is: feel free to make this less permissive, but let's make sure to do a full accounting of any differences in the resulting object before we merge.

[foolip]

As it is, the resulting objects are identical before and after this change, and also if #9842 is merged, my shorter and stricter extend produces the exact same result.

This should be confirmed once more before merging, of course.

[ddbeck]

@jonathantneal Thank you for opening this PR. This is pretty compelling (and reminds me that I've neglected some other dependent-friendly PRs 😬 ). I'm definitely drawn to things that are more explicit (and less peculiar). I'll respond to some of the line-comments shortly.

But first, to respond to @foolip's top-level question:

It would be great if the new extend could be tested, but to do that would require breaking it out into a helper that's not excluded by .npmignore, so I'll leave it to @ddbeck to say if that's worthwhile.

On this point, could we go a half-step and check-in the tests now, but not permanently wire them up? There are a couple of PRs to compile all the JSON down to a single file to shrink the package size (see #7374). If we did that after this PR, we could simplify index.js further and sidestep this whole .npmignore thing.

[foolip]

I love #7374 + #7398, I'd had the same thought but didn't know work had been done already. If we go that path, the code merging JSON files would be used at build time only and would be easily tested.

[bershanskiy]

I love this! Replacing over 23kB of unpacked node_modules with just 20 lines of code and less weirdness is great.

[jonathantneal]

I’ll be around in an hour to implement the requests.

[foolip]

I have confirmed again that this results in exactly the same object (if written to disk as JSON) as before. Suggested some additional simplification, but it's not super important.

I'll leave the final review of this to @ddbeck or @Elchi3.

[ddbeck]

Hi @jonathantneal, were you planning to come back to this PR?

[foolip]

@ddbeck I've applied my own suggestions now and think this is good to go without @jonathantneal making any further changes, if you want to give it a review.

[foolip]

I've also confirmed that the resulting data (serialized as JSON) is still exactly the same with this change, comparing commit 176d4ed and this PR merged into the same commit.

[jonathantneal]

Thanks, @foolip! 👍

[foolip]

Ping @ddbeck for review. If this goes stale, the work to validate that it doesn't change anything will have to be done again.

[ddbeck]

Thank you! 🎉

[ddbeck]

Actually, that wasn't enough: excellent work here, @jonathantneal and @foolip! I'm really pleased with what this achieves and the data problems it should prevent in the future. Well done!