Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Add custom type assertions for goog.is* functions. #886

Closed
wants to merge 1 commit into from

Conversation

ribrdb
Copy link

@ribrdb ribrdb commented May 29, 2019

Fixes #710

@rkirov
Copy link
Contributor

rkirov commented Jul 1, 2019

I am torn here, on one hand this is a good change for people that are still using goog.is*, on another hand I rather change gents to replace these with their actual implementations:

isBoolean -> typeof val == 'boolean'

which TypeScript has no problem of type narrowing from. I don't recommend anyone keep using goog.is* in their TS code.

@ribrdb
Copy link
Author

ribrdb commented Jul 1, 2019

Why not? We find the named functions easier to read than the actual implementation.
Especially isDefAndNotNull and isNull are much easier to tell apart than counting the number of = signs.

Also on a related note, I just discovered that goog.reflect.object also needs a custom type:

function object <T extends{[key:string]:any}>(type : {new(...args:any[]):{[P in keyof T]:any}}, object : T ) : T ;

The autogenerated one is completely useless. It requires casts to any use, which then removes all the type information that Closure Compiler needs for reflection.

@rkirov rkirov requested a review from shicks August 12, 2019 21:05
@rkirov
Copy link
Contributor

rkirov commented Aug 12, 2019

Let's get second opinion from @shicks. I think not having this special case will nudge people towards inlining goog.is* and friends which we think is beneficial and more idiomatic going forward.

@ribrdb
Copy link
Author

ribrdb commented Aug 13, 2019

Are you aware of anything like refasterjs for typescript to help migrate our existing code?
What do you think about adding a special case for goog.reflect.object? The currently generated type is just broken.

emit("(val : any ) : val is any[] ");
return true;
}
if (propName.equals("isBoolean")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actively working on deprecating and (eventually) deleting all of these. I'd rather not be encouraging their use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, I've got an LSC in progress to replace isString, isBoolean, isNumber, isDef, isNull, and isDefAndNotNull for all internal JS and TS code in one fell swoop.

I may look into doing the other three as well, and possible goog.typeof if I can figure out how to do it safely.

@rkirov
Copy link
Contributor

rkirov commented Aug 14, 2019

@ribrdb about global refactorings, we have been talking about the different options, but for now we just use TSLint with its autofixer API. Also, for these APIs something as simple as 'sed' might suffice.

@rkirov
Copy link
Contributor

rkirov commented Oct 24, 2019

Closing this as we are moving away from 'goog.is*' functions. I think something similar will be interesting to do once Type assertions land in 3.7 for Closure's assert methods.

@rkirov rkirov closed this Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clutz: output type guards for goog.base functions
4 participants