refactor(node-resolve): remove deep-freeze and deepmerge from dependencies

[alan-agius4]

This 2 dependencies are currently used to deepfreeze an exported symbol. Which seems to be an wasteful to add 2 extra dependencies for something trivial.

Also deep-freeze is licensed as Public Domain, which might be problematic for some 3rd parties such as the Angular CLI.

In Angular CLI we have a license validator that validates direct and transitive dependencies, and Public Domain is a problematic license becuse it falls under the "unencumbered' group which requires legal audit.

More context: https://opensource.google/docs/thirdparty/licenses/#unencumbered

[shellscape]

Alternative to adding a copy/convenience method to our code would be to reach out to substack to see if a more pleasing license could be applied.

deepmerge is a guard and unfortunately I won't approve removing that.

[shellscape]

I'd rather see a simple convenience method (const deepFreeze = () =>) in here to take the place of the removed dep on line 30, rather than a buncha calls to Object.freeze that we'd have to remember to add to anything new to defaults. A depth of 4 seems like it would be sufficient. The code is rather simple.

[TrySound]

Isn't it much easier to just duplicate defaults definition twice than adding a bunch of packages for very doubtful use case (laziness)?

[alan-agius4]

I was actually considering copy and pasting the defaults.

@shellscape, what do you think?

[shellscape]

Isn't it much easier to just duplicate defaults definition twice than adding a bunch of packages for very doubtful use case (laziness)?

man, I really take exception to that language.

@alan-agius4 I'd ask that you follow the guidance offered in my review

[TrySound]

By laziness I mean initial issue with adding ts extensions. It's really the only use case and requires two dependencies though it's not a big deal to list all necessary extensions in user config.

[shellscape]

deepmerge is a guard and unfortunately I won't approve removing that.

Sorry guys, not backing down from that.

[alan-agius4]

@shellscape, I added the deep freeze implementation.

[alan-agius4]

DeepFreeze package seems to be unmaintained. They do have a PR (https://github.com/substack/deep-freeze/pull/20 ) to change the license type but is staled.

[alan-agius4]

Can you explain what do you mean by deepmerge being a guard?

[shellscape]

Dude, please use the edit feature rather than three consecutive replies. No one likes their notifications to go bananas.

It's a guard to prevent mutating the exported defaults. Because this is compiled down to CJS, defaults could inadvertently be overwritten, causing program failure. Providing what is essentially a deep Object.assign copy of the object to the user prevents the plugin's defaults from being mutated even if they mess with the exports.

That code was put in place precisely because someone did exactly what is described above and encountered errors.

Please assume that we're not sitting around coming up with ways to be difficult. If a maintainer says it's a hard block and they aren't backing down, you can safely assume there's a good reason.

[shellscape]

blocked until deepMerge implementation is reinstated

[alan-agius4]

@shellscape, apologies for the extra notifications.

I believe that Object.freeze should be sufficient here. Also adding 2 extra dependencies for a single use case for defensive programming seems an overkill, but that’s just my opinion.

If it makes you happy and we can merge this PR I can add back deepmerge, but again it seem redundant to me considering you are freezing the object itself and all of it’s sub properties.

[shellscape]

I believe that Object.freeze should be sufficient here.

Unfortunately that's not true. This can be simply demonstrated.

// a.js
module.exports = { defaults: Object.freeze({ batman: 'batman' }) };

→ node 
Welcome to Node.js v12.18.1.
Type ".help" for more information.
> require('./a').defaults
{ batman: 'batman' }
> require('./a').defaults = null
null
> require('./a').defaults
null

[alan-agius4]

@shellscape, I see what you mean now. Thanks for the explanation.

But honestly if someone is doing that, they should be punished and not handle such cases. I cannot think of a genuine use case that it would be okay to that.

Also possible in this case a top level Object.assign({}, defaults) should suffice.

Ps: deepmerge re-implemented

[shellscape]

But honestly if someone is doing that, they should be punished and not handle such cases.

Thankfully, we have empathy for users in this project.

Change looks good. I'm on my phone right now and will merge when I return home.

[shellscape]

@alan-agius4 next time please use our Pull Request template. normally a script I run on my machine would have picked up that up and closed the PR citing the missing template. Guess you got lucky :)