babel-traverse module: explode function: ensure any prototype properties of cloned 'fns' object are retained

[jayaddison]

Q	A
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	No
Any Dependency Changes?	No
License	MIT

This change is a fixup and continuation of changes applied in #11811 to remove usages of the lodash/clone function.

Ref: #11811 (comment)

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/25804/

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cf01c15:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[nicolo-ribaudo]

Do we need to do this? I feel like the original behavior was just an unwanted side effect.

[jayaddison]

Hrm. On the one hand, mergePairs seems to operate only using enumerable keys when copying from a src object; I read that as a sign that the intent of the code is only to care about enumerable properties.

On the other hand, visitors is exported by the top-level babel-traverse module which could be an argument in favour of keeping things strictly the same behaviour-wise -- or it could be time for a wake-up call to anyone somehow relying on non-enumerable properties being exposed.

I'd probably be in favour of simplifying the set of properties (just in terms of simplicity and reducing undefined/unexpected behaviours outside the intent of the code) -- as long as that's not going to set half the JS ecosystem on fire.

cc @coreyfarrell just in case you missed any of this; this is a discussion around maintaining edge-case behaviour after replacing lodash/clone.

[coreyfarrell]

In the strictest view dropping the prototype could be a breaking change, but other interpretations could view it is a bug fix. I don't have a strong opinion about this patch being needed for v7, v8, both or neither. This edge case will not cause trouble for any babel plugin I've ever worked on. My primary concern was that the edge case be noticed so it can be decided what if anything to do. @jayaddison thanks for posting this PR!

[jayaddison]

Thanks @coreyfarrell!

If we're ok without needing this (at least until we discover any use cases where it does/could cause problems), then I think from a simplicity point of view it'd be preferable to close this. I think/hope we'd be able to reference and re-apply this later quickly if needed (hopefully there are enough relevant search terms in the GitHub discussion and topic.. I might add one or two more).