_.extend has been renamed

[samreid]

From #58:

Apparently _.extend is deprecated in the new lodash. It has been renamed to _.assignIn

it also looks like _.assign() may be appropriate for our use cases.

Flagging for developer meeting, but hopefully we can discuss/decide before then.

[pixelzoom]

As of 4.14.0, _.extend is still provided as an alias.

What has changed (as of 4.0.0) is that _.extend is now an alias of _.assignIn instead of _.assign.

[samreid]

We may also want to use _.assign, can someone volunteer to try it out and see what breaks?

[samreid]

@andrewadare volunteered to try out _.assign, but we should wait until we have the new version of lodash checked in to sherpa.

[samreid]

@pixelzoom will submit an issue to lodash asking about CDN copies of the new version.

[samreid]

@pixelzoom reported that the new CDN copies were available, just not linked.

EDIT: that issue is https://github.com/phetsims/sherpa/issues/58

[pixelzoom]

It looks like _.assign might be sufficient for PhET's needs. .assignIn adds some bells and whistles related to inherited properties, which are unnecessary for the pattern that we've been using for specifying default values for options.

[pixelzoom]

I'm also fine with continuing to use _.extends, since it's a supported alias (and unlikely to go away?)

[samreid]

.assignIn adds some bells and whistles related to inherited properties, which are unnecessary for the pattern that we've been using for specifying default values for options.

My guess is that if we are picking up any inherited properties, they are probably unintentional (and possibly buggy). Also assign may have (slightly?) higher performance since it isn't looking for inherited properties.

[jonathanolson]

We could potentially use PHET_CORE/extend, as I created it to basically be Underscore's extend, BUT with proper handling for ES5 getters (it copies the ES5 getter function, instead of Underscore's extend, which evaluated the getter and copied the value).

[samreid]

Are there cases where we are relying on extending with es5 getters? I'm wondering if it falls into the same case as unintentional and possibly buggy (as I surmised for extending inherited properties).

[pixelzoom]

@jonathanolson wrote:

We could potentially use PHET_CORE/extend,

I'm not excited about having to add this require statement to almost every source file:

var extend = require( 'PHET_CORE/extend' )

[samreid]

Brainstorming here: if we decide we like PHET_CORE/extend but dislike adding that require statement to every file we could add this code in initializeGlobals:

var extend = require( 'PHET_CORE/extend' );
window.phet.extend = extend;

Then instead of _.extend we would use phet.extend.

That being said, my vote is still for _.assign.

[jonathanolson]

Are there cases where we are relying on extending with es5 getters? I'm wondering if it falls into the same case as unintentional and possibly buggy (as I surmised for extending inherited properties).

We shouldn't be using _.extend with ES5 getters right now, but I haven't checked.

I'm not excited about having to add this require statement to almost every source file:

Sounds like an argument for making it a global instead if it needs to have broad usage. Is it worth revisiting things like PHET_CORE/inherit to see if we should have globals for that?

[samreid]

Is it worth revisiting things like PHET_CORE/inherit to see if we should have globals for that?

Yes, good point. It seems odd to have a different pattern for doing inherit vs extend. I'll create a PHET_CORE issue for discussion about inherit. Not sure if other phet methods should be global.

[andrewadare]

I have been assigned to test .assign, but this is blocked by #58. Un-assigning myself for now; please re-assign me if/when .assign should be tested in a new lodash library version.

[pixelzoom]

@samreid Since we've replaced _.extend with merge, can this issue be closed?

[jonathanolson]

Scenery examples and things use _.extend still. So doesn't look safe yet

[pixelzoom]

@jonathanolson The issue is not whether we're still using _.extend anywhere. This issue proposes that we evaluate replacing _.extends with _.assign. Do you plan to do that for "scenery examples and things"? It seems unnecessary to me.

[jonathanolson]

Do you plan to do that for "scenery examples and things"? It seems unnecessary to me.

If the current way continues to work, I'd like to leave it.

[samreid]

Even though _.extend is unlikely to ever be removed from the lodash API, it is preferable to query-replace our usages with _.assignIn so that we are using modern, idiomatic lodash. Additionally I suspect that some or many of these cases should be rewritten to use merge instead. Neither of these is high priority, but they would be a step in the right direction.

[pixelzoom]

OK, sounds like you want to keep this issue open then. The issue was created in July 2016. Can we agree to close if it's not addressed by 2024?

[samreid]

Sounds promising! Maybe we should have a rolling process where when each issue celebrates its 10th birthday, it gets special attention and/or closure. 🍰

[samreid]

We are down to 27 occurrences and I can see that all package.json refer to 4.0.0 or higher, so this seems safe to proceed. However, there are occurrences in Installer Builder and Yotta that I don't know how to test. So perhaps this should be a chip-away where responsible devs can test their own projects.

[samreid]

I updated some cases that I was capable of testing.

[samreid]

OK, sounds like you want to keep this issue open then. The issue was created in July 2016. Can we agree to close if it's not addressed by 2024?

Thanks for your patience, I think this issue can be closed. It's not too confusing to use _.extend instead of _.assignIn.