Use PHET_CORE/merge instead of _.extend

[zepumph]

This issue is to discuss and implement the conversion of our extending pattern of options from using _.extend() to merge(). At today's design pattern meeting, there were no objections towards moving to a "always use merge" pattern.

Action items:

• Make sure all developers know about this change.
• Update documentation to reflect this change in pattern.
• Investigate updating previous usages to use merge instead of _.extends.
  • The first here is a blanket conversion from _.extends to require( 'PHET_CORE/merge')
  • If there aren't many problems, then we can worry about making it right.

[zepumph]

I posted to the dev channel:

I'm working on #71, and RE: the first checkbox, here is your PSA:
In new code, use merge instead of _.extend in call cases where you can require it.

👍

[zepumph]

I made a way to automatically convert usages of _.extend to merge, including adding the import. It is convoluted, and probably wasn't worth the time, but I'm happy, because. . . well . . . automation.

Here are the steps to reproduce:

1. pull all repos

2. Make sure you have NO working copy changes in any of the repos you are touching.

3. go to chipper/eslint/eslintrc.js and turn the no-extend rule to 2 (error).

4. go to CHIPPER/lint.js and turn on the fix option, and uncomment the fix outputter

5. go active-runnables or your favorite data list, and edit the repos you want this to apply to.

6. Add the following script to a file called git/sandbox/js.js or another scrap file, just make just to change the path below. This script will add the merge require statement for each filepath in a file called test.txt.

   ( async () => {
   
      const cp = require( 'child_process' );
      const execute = require( '../perennial/js/common/execute' );
      const fs = require( 'fs' );
   
      const x = fs.readFileSync( 'test.txt', 'utf-8' ).trim().split( /\r?\n/ ).map( sim => sim.trim() );
   
      x.forEach( async file => {
   
        await new Promise( resolve => {
   
          const f = `${process.cwd()}\\${file.split( '/' ).join( '\\' )}`.split( '\\' ).join( '\\\\' );
          console.log( 'fixing', f );
   
          const s = cp.spawn( 'bash' );
          s.stdin.end( `grunt insert-require-statement --name=merge --file=${f}` );
          s.once( 'exit', () => {
            resolve();
          } );
        } );
   
      } );
      await execute( 'rm', [ 'test.txt' ], process.cwd() );
    } )();

7. Run for-each.sh perennial/data/active-runnables "grunt lint"

8. Run for-each.sh perennial/data/active-runnables "git diff --name-only --staged | xargs git diff --name-only > test.txt && node ../sandbox/js.js". This script is responsible for gathering a list of files that have working copy changes in them, and running grunt insert-require-statement on them to add merge (via the node script).

9. Run aqua, and make sure all is well.

I ran this process for the sims I am responsible for, mainly as a test, and things went very well. In fact, the validation in merge fixed one spot in FPAF where we were passing a faulty arg in (see commit above in WaterTowerScreenView).

I am not recommending that anyone else do these steps, but I thought it was important to keep good notes. Likely I will try to convert other repos until we can think of a reason not too (i.e. see #73).

[zepumph]

Also note that I added a lint rule for this purpose, which supports the "fix" option, but I don't necessarily think it should stick around.

[zepumph]

I updated documentation in phetsims/phet-info@0d075cb and ed3bb6b

[pixelzoom]

#73 is a prerequisite to global replacement of _.extend with merge.

[zepumph]

@pixelzoom, after looking through #73, does the global replacement seem good for you?

[pixelzoom]

yes, commented in the issue and closed it.

[zepumph]

In fact, the validation in merge fixed one spot in FPAF where we were passing a faulty arg in

Add one more to the bug catch count!

[pixelzoom]

That indicates that there is a missing test in mergeTest.js. Should we correct that?

[zepumph]

I do not think so. It was a faulty usage, not faulty merge code.

[pixelzoom]

#74 is a prerequisite to global replacement of _.extend with merge.

[zepumph]

I'm in the process of fixing all active runnables not already touched currently, and it is going well. The scripts above are doing their job, and I will just make sure to test well, and monitor CT this afternoon. Commits likely coming soon.

[zepumph]

There is a no-extend lint rule that isn't turned on. I'm not sure how hard it would be to get this going in our post es6-modules. I'll take a look.

[zepumph]

This was covered by the bad-sim-text lint rule, so I deleted it.

[zepumph]

All that is left is a review on the sim-specific cases.

[jessegreenberg]

I reviewed phetsims/capacitor-lab-basics@2493d0c and HingePointNode looks good in that sim.

Usages in ESP are also good. Closing.