Can merge overwrite `undefined` please?

[zepumph]

With typescript I think we can make an improvement to our merge implementation.

Here is an example using the current implementation:

const providedOptions = { something: undefined };

const options = phet.phetCore.merge( {
  something: 'has a value'
}, providedOptions );

console.log( options.something ); // undefined

I wish that it filled in 'has a value' for options.something.

Here is why it could be nice:

// Super class
type SuperOptions = { superOption: boolean };

class Super {
  constructor( providedOptions: SuperOptions ) {}
}

// Composed class
type SelfOptions = {
  x?: number,
  y?: number
};
type ComposedOptions = SelfOptions;

class Composed {
  constructor( providedOptions: ComposedOptions ) {
    // providedOptions.x was passed in as undefined

    const options = optionize<ComposedOptions, SelfOptions>( {
      x: 0,
      y: 0
    }, providedOptions );

    options.x; // UNDEFINED!!!!
  }
}

// Subclass
type SubclassOptions = SuperOptions & {};

class Subclass extends Super {
  constructor( providedOptions: SubclassOptions ) {
    const options = optionize<SubclassOptions, {}, SuperOptions>( {

      // Don't provide a default for superOption here

    }, providedOptions );

    // type is already boolean | undefined, why do I need to provide a default in the sub type
    const x = new Composed( {
      x: options.superOption // undefined
    } );
    super( options );
  }
}

Basically using composition in subtypes means you have to either redefine the default or accept that undefined may be the value after the composed type runs merge. This bites us all quite often, but until Typescript we always wanted to protect ourselves by assigning a default whereever options would be used, but now, we have type safety, and can use an option even if we didn't provide a default, and optionize will know that the type could be undefined.

Marking for dev meeting.

[samreid]

I don't fully understand the example above. Would be good to discuss further.

[zepumph]

Discussed at dev meeting today, we are all ok with overwriting the 'explicit' undefined. I'll implement. THanks!

[zepumph]

Fixed with a small line of code, and tested with this block in any typescript file (checking on options.x in the debugger pause).

// Super class
type SuperTestOptions = { superOption: number };

class SuperTest {
  constructor( providedOptions: SuperTestOptions ) {}
}

// Composed class
type ComposedSelfOptions = {
  x?: number;
  y?: number;
};
type ComposedOptions = ComposedSelfOptions;

class Composed {
  constructor( providedOptions?: ComposedOptions ) {
    // providedOptions.x was passed in as undefined

    const options = optionize<ComposedOptions, ComposedSelfOptions>()( {
      x: 0,
      y: 0
    }, providedOptions );
debugger;
    options.x; // UNDEFINED!!!!
  }
}

// Subclass
type SubclassOptions = SuperTestOptions & {};

class Subclass extends SuperTest {
  constructor( providedOptions?: SubclassOptions ) {
    const options = optionize<SubclassOptions, {}, SuperTestOptions>()( {

      // Don't provide a default for superOption here

    }, providedOptions );

    // type is already boolean | undefined, why do I need to provide a default in the sub type
    const myComposed = new Composed( {
      x: options.superOption // undefined
    } );
    super( options );
  }
}

const hi = new Subclass();

[samreid]

Seeing the code change https://github.com/phetsims/phet-core/commit/b19e92e592d31a28088973ed8f261d8de83698e8 made this issue much clearer to me. Good change, thanks!

[samreid]

I should also mention that the code change is more aligned with the merge typescript return types.