Skip to content

add types for escape and unescape methods #18813 #19015

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 3 commits into from
Nov 13, 2017
Merged

Conversation

wbhob
Copy link
Contributor

@wbhob wbhob commented Oct 8, 2017

Although the issue is marked working as expected, it is important to mention that most major browsers maintain support for escape and unescape, and some javascript codebases moving to typescript may have escape and unescape in them. They are valid JavaScript, and thus they should be included in the global definition.

Fixes #18813

wbhob added 2 commits October 7, 2017 23:24
although the issue is marked working as expected, it is important to mention that most major browsers maintain support for escape and unescape, and some javascript codebases moving to typescript may have escape and unescape in them. They are valid JavaScript, and thus they should be included in the global definition.
@msftclas
Copy link

msftclas commented Oct 8, 2017

CLA assistant check
All CLA requirements met.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 9, 2017

Thanks for the contribution. but the omission of these functions from the library is intentional. please see #18813 (comment)

@mhegazy mhegazy closed this Oct 9, 2017
@akehir
Copy link

akehir commented Oct 16, 2017

@mhegazy - In the current ES Draft spec, as well as in MDN I see no reference to escape / unescape being deprecated anymore. What is your reference for it still being deprecated?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 16, 2017

u are right. there does not seem to be a deprecation notice any longer.

@mhegazy mhegazy reopened this Oct 16, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Nov 9, 2017

@wbhob mind refreshing the PR and addressing the failing tests?

@@ -1,14 +1,14 @@
/*! *****************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

do not change this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that was automatically generated

@wbhob
Copy link
Contributor Author

wbhob commented Nov 9, 2017

I'll look into fixing those tests soon

@mhegazy mhegazy merged commit c2f0c58 into microsoft:master Nov 13, 2017
@wbhob
Copy link
Contributor Author

wbhob commented Nov 13, 2017

🎉 Yay! Thank you

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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