Skip to content

This parameters in lib #9688

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

Closed
wants to merge 10 commits into from
Closed

This parameters in lib #9688

wants to merge 10 commits into from

Conversation

sandersn
Copy link
Member

Fixes #9654

Adds parameters for all the primitives, not just String.

Unfortunately, 376 additions, 373 deletions is too many for github to show. So you can't actually see the changes themselves in the web UI.

@sandersn
Copy link
Member Author

sandersn commented Jul 13, 2016

Looks like Codeflow will work for the Microsoft-internal folks. You just have to install Codeflow's Chrome extension.

@weswigham
Copy link
Member

@sandersn Shouldn't all the subclassable interfaces (ie, Array<T>) have this function types of this: this? Array is already generic, so there's minimal opportunity cost to it.

@sandersn
Copy link
Member Author

Unfortunately, switching to this: this exposes a bug with assignability checking for (I believe) types that have lots of methods with this: this. I don't think it is specific to Array necessarily.

I'll open an issue on it when I get a minimal repro. Until then I'll leave the PR the way it is.

@sandersn
Copy link
Member Author

I opened #9707, but I didn't get the bug much smaller.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 13, 2016

@sandersn i would say let's close this one until we figure out #9707. i do agree we need to fix that one.

@sandersn
Copy link
Member Author

#9707 is actually fixed. I just didn't have time to come back to this PR.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 13, 2016

Ah. i actually was thinking of #10288, sorry about that.

but now that i am looking at the right bug, I assume no harm comes from this change.

@sandersn
Copy link
Member Author

Unfortunately, it's not done. @weswigham pointed out that Array<T> should have this: this, so I still need to do that before it's finished.

@sandersn
Copy link
Member Author

Actually, I think I misunderstood. Yes, even without fixing #10288, adding more this annotations is useful. (It would be more useful if we could fix #10288.)

@sandersn
Copy link
Member Author

@mhegazy this change is finished now that I added this: this for Array and Object. There's no performance impact, as measured by running the Monaco and TFS performance baselines.

@mhegazy
Copy link
Contributor

mhegazy commented May 23, 2017

Based on conversation with @sandersn, this change has little value, but introduces a new set of complexities to the library and checking the library. value does not offset the cost.

@mhegazy mhegazy closed this May 23, 2017
@mhegazy mhegazy deleted the this-parameters-in-lib branch August 23, 2017 19:57
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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.

4 participants