fix(core): allow undefined value for renderer setElement(…)

[Manduro]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Renderer (V1) issue #13686, which has been closed but not fixed.

What is the new behavior?
Using Renderer’s setElementAttribute or setElementStyle with a null or undefined value removes the
corresponding attribute or style. The argument type now allows this when using strictNullChecks.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

Other information:

[chuckjaz]

Note this is a breaking change and should be held until the first v5 beta.

It is not breaking for callers of callers of the render API but it is breaking for the implementors as they need now need to change the type of the third parameter to a union with undefined.

[chuckjaz]

Consider changing the commit message to fix(core): allow undefined value for renderer setElement(…) as making a parameter adds undefined to the type not null.

[Manduro]

@chuckjaz I'm not sure if this really is a breaking change. Can you provide an example where existing code would break?

Both functions where I'm changing the signature have checks for null/undefined's: here and here.
The problem is that when using strictNullChecks it does not allow that last parameter to be undefined. So imo this actually adds back functionality, instead of breaking anything.

[chuckjaz]

@Manduro I removed the breaking change tag. It turns out that TypeScript is tolerant of incorrectly typed overrides.

The issue I had is with implementations of Renderer. Consider the following:

abstract class Base {
  abstract setElementAttribute(renderElement: any, attributeName: string, attributeValue: string): void;
}

class Derived extends Base {
  setElementAttribute(renderElement: any, attributeName: string, attributeValue: string): void { }
}

Changing attributeValue to be attributeValue?: string in the Base class changes the contract on what Derived can be called with. For example, the following type-checks:

const b: Base = new Derived();
b.setElementAttribute(1, 'attr');

which will pass undefined as attributeValue to Deriveds setElementAttribute even though it declared attributeValue to be a non-nullable string. Correctly typed overrides would require that all derived types of Base (hence Renderer) would need to be updated.

It appears that TypeScript is tolerant of this kind of incorrect override declaration so this doesn't break and we can take this change because all implementation were written prior to strict null checking and wouldn't have relied on attributeValue always being a string.

[Manduro]

Ah yes, hadn't considered that. Glad it's not a problem.

I'm not touching Renderer2 by the way, to be clear. It has separate methods for adding and removing attributes and styles.

[chuckjaz]

RE: Renderer2, updated my comment above to change Renderer2 to Renderer.

[splincode]

@Manduro what is the status of your pull request? is it still relevant?

[Manduro]

@splincode I guess it is as renderer v1 is still in the codebase, although it might not be long before it's removed. Don't know why this was never merged.

[mhevery]

http://test/OCL:216772826:BASE:216772853:1539297379895:ebc14d26

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}