Skip to content

Fix WeakSet interface #19756

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

Merged
merged 1 commit into from
Jan 10, 2018
Merged

Fix WeakSet interface #19756

merged 1 commit into from
Jan 10, 2018

Conversation

falsandtru
Copy link
Contributor

@falsandtru falsandtru commented Nov 6, 2017

Fixes #14840

cc @Andy-MS

@ghost
Copy link

ghost commented Nov 6, 2017

@mhegazy @rbuckton Any idea what PR fixed it so you could have a one declaration one a type parameter constraint and one without?

@falsandtru
Copy link
Contributor Author

You mean lodash definitions?

@ghost
Copy link

ghost commented Nov 6, 2017

I meant a pull request to the TypeScript compiler, that would allow the lib.d.ts definition to use WeakSet<T extends object> and lodash to use WeakSet<T>. Just making sure we've officially fixed this.

@falsandtru
Copy link
Contributor Author

Sorry, I missed to find WeakSet because page loading is not complete. I'll fix lodash definitions.

@ghost
Copy link

ghost commented Nov 6, 2017

That may cause errors for people using lodash with earlier TypeScript versions though. We have no way of changing these both at the same time unfortunately. So it would be best to support backwards-compatibility so lib.d.ts could declare a constraint but lodash wouldn't have to.

@falsandtru
Copy link
Contributor Author

I made DefinitelyTyped/DefinitelyTyped#21273. This patch don't have unfavorite incompatibility.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 6, 2017

I think we need a fix to allow defaults on only one declaration as @Andy-MS noted in #19756 (comment)

@falsandtru
Copy link
Contributor Author

DefinitelyTyped/DefinitelyTyped#21273 is merged. So we can merge this PR.

@ghost
Copy link

ghost commented Nov 10, 2017

The problem is with interface WeakSet<T> { } at the bottom of lodash/index.d.ts. If you change that to interface WeakSet<T extends object> { } right now you'll get a compile error. But if this PR went in then it would have to be written that way to stay compatible, but then it wouldn't work on old TypeScript versions.

@falsandtru
Copy link
Contributor Author

Seems like it is impossible to fix WeakSet interface without breaking changes. We can never fix WeakSet?

@mhegazy
Copy link
Contributor

mhegazy commented Nov 28, 2017

We need a fix for #20018 first.

@falsandtru
Copy link
Contributor Author

Now can you merge?

@mhegazy mhegazy merged commit da593ca into microsoft:master Jan 10, 2018
@falsandtru falsandtru deleted the lib/weakset branch January 10, 2018 06:07
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants