Skip to content
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

lodash - change weakmap declaration to work with TS 2.2 #14662

Merged
merged 2 commits into from
Feb 24, 2017

Conversation

budiadiono
Copy link
Contributor

@budiadiono budiadiono commented Feb 16, 2017

In TS 2.2 WeakMap interface for ES6 has been changed from WeakMap<K, V> to WeakMap<K extends object, V>. This changes makes @types/lodash not working with new ES6 introduced in TS2.2.

To keep the backward compatibility with ES5, I think it is safe to rename the WeakMap declaration.

I know TS 2.2 is still in RC version now, but many people using it, including me :). So if you think this is too early and also bad idea then feel free to close this PR without merging it.

If this get merged then it's should close #14324.

This pull request is clean version of pull request #14502

Please fill in this template.

  • Make your PR against the master branch.
  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run tsc without errors.
  • Run npm run lint package-name if a tslint.json is present.

Select one of these and delete the others:

If changing an existing definition:

@dt-bot
Copy link
Member

dt-bot commented Feb 16, 2017

lodash/index.d.ts

to authors (@bczengel @chrootsu @stepancar). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@TonyPythoneer
Copy link
Contributor

TonyPythoneer commented Feb 16, 2017

@budiadiono I appreciate your idea but it's too early. It's because TypeScript hasn't released stable 2.2 version yet.
I hope this proposal can be an issue and make up a TODO list. It doesn't make you work in vain.

@corbinu
Copy link

corbinu commented Feb 22, 2017

Can this be merged now that 2.2 is stable?

@Dirrk
Copy link
Contributor

Dirrk commented Feb 22, 2017

Builds breaking waiting for this change, 2.2 is now live

@chrootsu
Copy link
Contributor

👍

@sshquack
Copy link

sshquack commented Feb 23, 2017

Thanks for the fix @budiadiono

Just to echo what others have already mentioned here, this problem is causing issues with the TypeScript 2.2 release

microsoft/TypeScript#14256

@@ -12701,7 +12701,7 @@ declare namespace _ {
* @param value The value to check.
* @returns Returns true if value is correctly classified, else false.
*/
isWeakMap<K, V>(value?: any): value is WeakMap<K, V>;
isWeakMap<K, V>(value?: any): value is WeakMapLike<K, V>;
Copy link
Contributor

Choose a reason for hiding this comment

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

this change disables the guard. WeakMapLike does not even have the declarations of WeakMap, so it is not usable.

I would say just remove WeakMap declaration altogether, and make this function return boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mhegazy would #14787 be a better solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think so. this change seems good to me, but we just need to remove the weakMap reference altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhegazy thanks for the review. I'll update it now.

@ghost
Copy link

ghost commented Feb 24, 2017

A better fix would be to have isWeakMap(value: any): value is WeakMap<any, any>;.
It doesn't make sense to have type parameters because generics are erased, and isWeakMap<{ x: number }, boolean>(foo) would still return true even if foo is a WeakMap<{ y: string }, Date>.
(It also doesn't make sense to have an optional parameter, because no one would ever call isWeakMap() with no parameters.)

Then the test must be changed to use e.g. WeakMap<{ x: number }, number> because of the type constraint on WeakMaps.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 24, 2017

A better fix would be to have isWeakMap(value: any): value is WeakMap<any, any>;.

@andy-ms, the issue is not the user-defined type guard, the issue is the declaration of WeakMap. WeakMap is not defined in lib.d.ts (only lib.es2015.collections.d.ts or one of its dependents); so lodash had a forward declaration of it. TS 2.2 changed the declaration to include T extends object on the key, now lodash needs to do the same, but object is a TS 2.2 feature. so you see it is a catch-22.

Ultimiatlly #14787 is the correct fix, but this unblocks ppl at the moment.

@mhegazy mhegazy merged commit 3775dcf into DefinitelyTyped:master Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lodash All declarations of 'WeakMap' must have identical type parameters.
9 participants