New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Update types for TypeScript nullability support #15405

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mhevery
Member

mhevery commented Mar 22, 2017

Fixes #15432

@googlebot googlebot added the cla: yes label Mar 22, 2017

@mhevery mhevery requested a review from IgorMinar Mar 23, 2017

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Mar 23, 2017

Member

@IgorMinar please look at the API guard only.

Member

mhevery commented Mar 23, 2017

@IgorMinar please look at the API guard only.

@@ -360,15 +364,15 @@ export class TimelineBuilder {
private _updateStyle(prop: string, value: string|number) {
this._localTimelineStyles[prop] = value;
this._globalTimelineStyles[prop] = value;
this._globalTimelineStyles ![prop] = value;

This comment has been minimized.

@vicb

vicb Mar 23, 2017

Contributor

is this clang formatting ?
have you created an issue for it (may be check the latest version before as we are far behind)

@vicb

vicb Mar 23, 2017

Contributor

is this clang formatting ?
have you created an issue for it (may be check the latest version before as we are far behind)

This comment has been minimized.

@mhevery
@mhevery
const type = record['type'];
const data = record['data'];
const startTime = record['startTime'];
const endTime = record['endTime'];
if (type === 'FunctionCall' && (data == null || data['scriptName'] !== 'InjectedScript')) {
events.push(createStartEvent('script', startTime));
events !.push(createStartEvent('script', startTime));

This comment has been minimized.

@vicb

vicb Mar 23, 2017

Contributor

not inferred here ?

@vicb

vicb Mar 23, 2017

Contributor

not inferred here ?

This comment has been minimized.

@mhevery

mhevery Mar 23, 2017

Member

Yes, it does not infer it.

@mhevery

mhevery Mar 23, 2017

Member

Yes, it does not infer it.

@@ -89,7 +89,7 @@ export function createPlatform(injector: Injector): PlatformRef {
}
_platform = injector.get(PlatformRef);
const inits = injector.get(PLATFORM_INITIALIZER, null);
if (inits) inits.forEach(init => init());
if (inits) inits.forEach((init:()=>void) => init());

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

Is this needed?

@tbosch

tbosch Mar 24, 2017

Member

Is this needed?

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

reverted

@mhevery

mhevery Mar 24, 2017

Member

reverted

@@ -99,8 +99,8 @@ export function createPlatform(injector: Injector): PlatformRef {
* @experimental APIs related to application bootstrap are currently under review.
*/
export function createPlatformFactory(
parentPlatformFactory: (extraProviders?: Provider[]) => PlatformRef, name: string,
providers: Provider[] = []): (extraProviders?: Provider[]) => PlatformRef {
parentPlatformFactory: ((extraProviders?: Provider[]) => PlatformRef) | null | undefined,

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

when would it be undefined?

@tbosch

tbosch Mar 24, 2017

Member

when would it be undefined?

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

removed undefined

@mhevery

mhevery Mar 24, 2017

Member

removed undefined

@@ -241,7 +241,7 @@ export declare abstract class ComponentFactory<C> {
/** @stable */
export declare abstract class ComponentFactoryResolver {
abstract resolveComponentFactory<T>(component: Type<T>): ComponentFactory<T>;
abstract resolveComponentFactory<T>(component: Type<T>): ComponentFactory<T> | null;

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

This is never null, as it throws otherwise.

@tbosch

tbosch Mar 24, 2017

Member

This is never null, as it throws otherwise.

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

reverted

@mhevery

mhevery Mar 24, 2017

Member

reverted

/** @stable */
export declare const CUSTOM_ELEMENTS_SCHEMA: SchemaMetadata;
/** @experimental */
export declare class DebugElement extends DebugNode {
attributes: {
[key: string]: string;
[key: string]: string | null | undefined;

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

When would the value be undefined?

@tbosch

tbosch Mar 24, 2017

Member

When would the value be undefined?

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

removed undefined

@mhevery

mhevery Mar 24, 2017

Member

removed undefined

@@ -310,7 +310,7 @@ export declare class DebugElement extends DebugNode {
[key: string]: any;
};
styles: {
[key: string]: string;
[key: string]: string | null | undefined;

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

When would the value be undefined?

@tbosch

tbosch Mar 24, 2017

Member

When would the value be undefined?

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

removed undefined

@mhevery

mhevery Mar 24, 2017

Member

removed undefined

@@ -730,7 +730,7 @@ export declare abstract class PlatformRef {
/** @experimental */
export interface Predicate<T> {
(value: T, index?: number, array?: T[]): boolean;

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

Isn't this breaking, i.e. changing this from optional to required?

@tbosch

tbosch Mar 24, 2017

Member

Isn't this breaking, i.e. changing this from optional to required?

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

discussed with @tbosch its OK.

@mhevery

mhevery Mar 24, 2017

Member

discussed with @tbosch its OK.

@@ -822,31 +822,31 @@ export declare abstract class Renderer2 {
readonly abstract data: {
[key: string]: any;
};
destroyNode: (node: any) => void | null;
destroyNode: ((node: any) => void) | null;

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

This seems wrong, we should never destroy a null node...

@tbosch

tbosch Mar 24, 2017

Member

This seems wrong, we should never destroy a null node...

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

correct

@mhevery

mhevery Mar 24, 2017

Member

correct

@@ -891,7 +891,7 @@ export declare abstract class RootRenderer {
/** @stable */
export declare abstract class Sanitizer {
abstract sanitize(context: SecurityContext, value: string): string;
abstract sanitize(context: SecurityContext, value: string): string | null;

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

This seems wrong. Either the value is also |null as the result, or none of them is...

@tbosch

tbosch Mar 24, 2017

Member

This seems wrong. Either the value is also |null as the result, or none of them is...

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

reverted

@mhevery

mhevery Mar 24, 2017

Member

reverted

getAllRootElements(): any[];
getAllTestabilities(): Testability[];
getTestability(elem: any): Testability;
getTestability(elem: any): Testability | undefined;

This comment has been minimized.

@tbosch

tbosch Mar 24, 2017

Member

This should be |null, or at least match findTestabilityInTree

@tbosch

tbosch Mar 24, 2017

Member

This should be |null, or at least match findTestabilityInTree

This comment has been minimized.

@mhevery

mhevery Mar 24, 2017

Member

fixed

@mhevery

mhevery Mar 24, 2017

Member

fixed

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch Mar 24, 2017

Member

Reviewed the changes to core.d.ts

Member

tbosch commented Mar 24, 2017

Reviewed the changes to core.d.ts

@@ -299,7 +299,7 @@ export class PlatformRef_ extends PlatformRef {
throw new Error('No ErrorHandler. Is platform module (BrowserModule) included?');
}
moduleRef.onDestroy(() => remove(this._modules, moduleRef));
ngZone.onError.subscribe({next: (error: any) => { exceptionHandler.handleError(error); }});
ngZone !.onError.subscribe({next: (error: any) => { exceptionHandler.handleError(error); }});

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

should we introduce a new variable when TS is not able to infer that the value is never null to avoid using ! too often ?

@vicb

vicb Mar 24, 2017

Contributor

should we introduce a new variable when TS is not able to infer that the value is never null to avoid using ! too often ?

@@ -29,18 +29,18 @@ export class DebugNode {
this.listeners = [];
}
get injector(): Injector { return this._debugContext ? this._debugContext.injector : null; }
get injector(): Injector { return this._debugContext.injector; }

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

not sure about this ? IIRC @tbosch added this to fix an issue ?

@vicb

vicb Mar 24, 2017

Contributor

not sure about this ? IIRC @tbosch added this to fix an issue ?

@@ -56,7 +56,7 @@ export class CodegenComponentFactoryResolver implements ComponentFactoryResolver
resolveComponentFactory<T>(component: {new (...args: any[]): T}): ComponentFactory<T> {
let factory = this._factories.get(component) || this._parent.resolveComponentFactory(component);
return factory ? new ComponentFactoryBoundToModule(factory, this._ngModule) : null;
return new ComponentFactoryBoundToModule(factory, this._ngModule);

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

this just probably return | null might be an issue with the current code, will check

@vicb

vicb Mar 24, 2017

Contributor

this just probably return | null might be an issue with the current code, will check

@@ -60,7 +60,7 @@ export class NgModuleFactory<T> {
get moduleType(): Type<T> { return this._moduleType; }
create(parentInjector: Injector): NgModuleRef<T> {
create(parentInjector?: Injector|null): NgModuleRef<T> {

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

Should not need ?

@vicb

vicb Mar 24, 2017

Contributor

Should not need ?

@@ -55,7 +55,7 @@ export function createScope(signature: string, flags: any = null): any {
export function leave<T>(scope: Scope, returnValue?: T): T {
trace.leaveScope(scope, returnValue);
return returnValue;
return returnValue !;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

Does not seem right. return |undefined ?

@vicb

vicb Mar 24, 2017

Contributor

Does not seem right. return |undefined ?

@@ -30,5 +30,5 @@ export enum SecurityContext {
* @stable
*/
export abstract class Sanitizer {
abstract sanitize(context: SecurityContext, value: string): string;
abstract sanitize(context: SecurityContext, value: string): string|null;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

hum ?

@vicb

vicb Mar 24, 2017

Contributor

hum ?

@@ -136,13 +136,13 @@ export class TestabilityRegistry {
this._applications.set(token, testability);
}
getTestability(elem: any): Testability { return this._applications.get(elem); }
getTestability(elem: any): Testability|undefined { return this._applications.get(elem); }

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

update for |null ?

@vicb

vicb Mar 24, 2017

Contributor

update for |null ?

if (fnOrArray === Object || fnOrArray === String || fnOrArray === Function ||
fnOrArray === Number || fnOrArray === Array) {
throw new Error(`Can not use native ${stringify(fnOrArray)} as constructor`);
}
if (typeof fnOrArray === 'function') {
return fnOrArray;
return fnOrArray as Function;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

needed ? TS should infer here

@vicb

vicb Mar 24, 2017

Contributor

needed ? TS should infer here

@@ -96,18 +96,18 @@ function extractAnnotation(annotation: any): any {
return annotation;
}
function applyParams(fnOrArray: (Function | any[]), key: string): Function {
function applyParams(fnOrArray: Function | any[] | undefined, key: string): Function {

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

hum ?

@vicb

vicb Mar 24, 2017

Contributor

hum ?

@@ -337,7 +337,7 @@ export function makeParamDecorator(
// there might be gaps if some in between parameters do not have annotations.
// we pad with nulls.
while (parameters.length <= index) {
parameters.push(null);
parameters.push(null !);

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

hum ?

@vicb

vicb Mar 24, 2017

Contributor

hum ?

let ns: string;
let name: string;
let ns: string = null !;
let name: string = null !;
if (namespaceAndName) {
[ns, name] = splitNamespace(namespaceAndName);

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

@tbosch could we have const [ns, name] = splitNamespace(namespaceAndName); here and move this closer to where this is used ... much below ?

@vicb

vicb Mar 24, 2017

Contributor

@tbosch could we have const [ns, name] = splitNamespace(namespaceAndName); here and move this closer to where this is used ... much below ?

elDef = viewParentEl(view);
view = view.parent;
elDef = viewParentEl(view) !;
view = view.parent !;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

while(view)

@vicb

vicb Mar 24, 2017

Contributor

while(view)

@@ -135,7 +135,7 @@ class ViewContainerRef_ implements ViewContainerData {
let elDef = this._elDef.parent;
while (!elDef && view) {
elDef = viewParentEl(view);
view = view.parent;
view = view.parent !;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

while (view)

@vicb

vicb Mar 24, 2017

Contributor

while (view)

if (nodeIndex == null) {
this.nodeIndex = nodeIndex = 0;
}
this.nodeDef = view.def.nodes[nodeIndex];
let elDef = this.nodeDef;
let elView = view;
while (elDef && (elDef.flags & NodeFlags.TypeElement) === 0) {
elDef = elDef.parent;
elDef = elDef.parent !;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

while

@vicb

vicb Mar 24, 2017

Contributor

while

elDef = viewParentEl(elView);
elView = elView.parent;
elDef = viewParentEl(elView) !;
elView = elView.parent !;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

while

@vicb

vicb Mar 24, 2017

Contributor

while

@@ -73,9 +73,9 @@ export function resolveRendererType2(type: RendererType2): RendererType2 {
}
}
if (type && type.id === EMPTY_RENDERER_TYPE_ID) {
type = null;
type = null !;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

return null

@vicb

vicb Mar 24, 2017

Contributor

return null

if (viewIndex == null || viewIndex >= embeddedViews.length) {
viewIndex = embeddedViews.length - 1;
}
if (viewIndex < 0) {
return null;
}
const view = embeddedViews[viewIndex];
view.viewContainerParent = undefined;
view.viewContainerParent = null;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

@tbosch IIRC there's one place where we explicitly want both null and undefined, could you check if it has not been changed by this PR

@vicb

vicb Mar 24, 2017

Contributor

@tbosch IIRC there's one place where we explicitly want both null and undefined, could you check if it has not been changed by this PR

@@ -131,7 +131,7 @@ export class DatePipe implements PipeTransform {
if (!isDate(date)) {
let match: RegExpMatchArray;
if ((typeof value === 'string') && (match = value.match(ISO8601_DATE_REGEX))) {
if ((typeof value === 'string') && (match = value.match(ISO8601_DATE_REGEX) !)) {

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

hum ?

@vicb

vicb Mar 24, 2017

Contributor

hum ?

@@ -164,7 +164,7 @@ function createPositionApplyingDoubleDots(
let dd = numberOfDoubleDots;
while (dd > ci) {
dd -= ci;
g = g.parent;
g = g.parent !;
if (!g) {

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

hum

@vicb

vicb Mar 24, 2017

Contributor

hum

@@ -71,8 +71,8 @@ export class RouterOutlet implements OnDestroy {
if (!this.activated) throw new Error('Outlet is not activated');
this.location.detach();
const r = this.activated;
this.activated = null;
this._activatedRoute = null;
this.activated = null !;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

type should be nullable

@vicb

vicb Mar 24, 2017

Contributor

type should be nullable

@@ -37,13 +37,13 @@ export abstract class RouteReuseStrategy {
abstract shouldDetach(route: ActivatedRouteSnapshot): boolean;
/** Stores the detached route */
abstract store(route: ActivatedRouteSnapshot, handle: DetachedRouteHandle): void;
abstract store(route: ActivatedRouteSnapshot, handle: DetachedRouteHandle|null): void;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

This should not be null

@vicb

vicb Mar 24, 2017

Contributor

This should not be null

readonly pathFromRoot: ActivatedRoute[];
readonly queryParamMap: Observable<ParamMap>;
queryParams: Observable<Params>;
readonly root: ActivatedRoute;
readonly routeConfig: Route;
readonly routeConfig: Route | null;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

should not be null

@vicb

vicb Mar 24, 2017

Contributor

should not be null

readonly pathFromRoot: ActivatedRouteSnapshot[];
readonly queryParamMap: ParamMap;
queryParams: Params;
readonly root: ActivatedRouteSnapshot;
readonly routeConfig: Route;
readonly routeConfig: Route | null;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

should not be null

@vicb

vicb Mar 24, 2017

Contributor

should not be null

abstract shouldAttach(route: ActivatedRouteSnapshot): boolean;
abstract shouldDetach(route: ActivatedRouteSnapshot): boolean;
abstract shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean;
abstract store(route: ActivatedRouteSnapshot, handle: DetachedRouteHandle): void;
abstract store(route: ActivatedRouteSnapshot, handle: DetachedRouteHandle | null): void;

This comment has been minimized.

@vicb

vicb Mar 24, 2017

Contributor

should not be null

@vicb

vicb Mar 24, 2017

Contributor

should not be null

@mhevery mhevery closed this Apr 10, 2017

@mhevery mhevery deleted the mhevery:nullability branch Jun 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment