phetsims / tasks

General tasks for PhET that will be tracked on Github
MIT License
5 stars 2 forks source link

Prefer `_.assign` to `_.extend()` and `_.assignIn()`? #1130

Closed zepumph closed 11 months ago

zepumph commented 11 months ago

Recently, webstorm has been complaining about how _.extend() in typescript has a void return type. In lodash doc it looks to be an alias for _.assignIn(). And so I recommend that = _.extend( becomes bad text.

Proposal 1 We recommend and rename to _.assignIn(), the alias.

Proposal 2 We in fact recommend _.assign instead, because it doesn't apply to inhereted prototype properties. That seems a bit safer to me:

From the doc for _.assignIn in https://lodash.com/docs/4.17.15#assign:

This method is like _.assign except that it iterates over own and inherited source properties.

Term Usages
= _.assignIn( 6
_.assignIn( 14
= _.extend( 25
_.extend( 55
= _.assign( 0
_.assign( 3

(searching for all but .md files)

To me .assign seems like the right term. Looking through our usages of `.extendand_.assignIn` and I can't see any cases in which objects should have a prototype or it would be expected to support type hierarchy. @samreid, what do you think we should do here??

I recommend using _.assign in all cases and disallowing the other two from our code base.

samreid commented 11 months ago

For our typical usage of assign/assignIn/extends when it comes to options or object literals, I think they would have similar behavior. However, I would be surprised if our codebase isn't running other object types (with prototypes) through assign/assignIn. So I would recommend running an experiment where we create our own assignment function myAssign that checks for own/source/inherited properties and throws an assertion or console.logs if there is a difference (GPT4 can write a rough draft for this if you want). We may have some cases where we are supposed to check own/source/prototype/inherited properties and may get a runtime failure if just switching to assign for those cases. For other cases, we will have to decide whether to use _.assign, or use our safeguarded myAssign or just use Object.assign. It's not very clear to me which to prefer or how we will decide.

zepumph commented 11 months ago

However, I would be surprised if our codebase isn't running other object types (with prototypes) through assign/assignIn.

I was surprised too, but nothing I saw had anything with a prototype. It was all object literals. I'll definitely make sure to do a good double check. In sim code we can feel more confident through. I'll start hacking away at this to get it consistent.

zepumph commented 11 months ago

assignIn seems preferable and more clear when using it with an equals sign. I added it to bad text, and I'm ready to move on. I don't think we need to devote much energy to this issue, I just want to avoid the webstorm flag when possible. Closing