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

Add shim for ES Promise #33863

Closed
wants to merge 2 commits into from
Closed

Add shim for ES Promise #33863

wants to merge 2 commits into from

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Oct 7, 2019

This PR adds a shim for the ES Promise, which is needed for some performance-related future work.

@rbuckton rbuckton requested review from weswigham and amcasey October 7, 2019 23:44
race<T>(promises: (T | PromiseLike<T>)[]): Promise<T>;
}

export interface Promise<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I, for one, look forward to Promise is not compatible with ts.Promise errors. 😄

This feels like a good time to just bump the lib version to es6 and shim runtime things as needed, rather than adding blocks of stuff onto the es5 lib.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be fine with having ts.Promise depend on the global native type information for Promise. What I don't want to do is have our shims insert themselves as the global implementations, as that could cause conflicts with other packages.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. I definitely just want to use the global Promise types, though.

@AviVahl
Copy link

AviVahl commented Oct 8, 2019

Just thought I'll drop an outside opinion:

Are all theses inline shims really necessary? Supported Node versions already have Promise/Map/etc implemented. If older web browsers are the concern, wouldn't it be enough to document environment requirements in the README and let package consumers worry about the polyfills? There are quite elegant solutions today, such as polyfill.io.

Writing this because your time and work are precious to us, the community. :)

@j-oliveras
Copy link
Contributor

@AviVahl I think that typescript must work with IE (see last paragraph on #33771).

@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
Copy link
Contributor

@ExE-Boss ExE-Boss left a comment

Choose a reason for hiding this comment

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

The spec uses:

  1. If Type(value) is not Object, then
    1. Return FulfillPromise(promise, value).

The current implementation is closer to:

  1. If Type(value) is not Object or IsCallable(value) is true, then
    1. Return FulfillPromise(promise, value).

try {
if (promise === value) throw new TypeError();
// eslint-disable-next-line no-null/no-null
const then = typeof value === "object" && value !== null && (<Promise<unknown>>value).then;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const then = typeof value === "object" && value !== null && (<Promise<unknown>>value).then;
const then = ((typeof value === "object" && value !== null) || typeof value === "function") && (<Promise<unknown>>value).then;

@sandersn
Copy link
Member

sandersn commented Mar 4, 2020

@rbuckton is this still needed?

@amcasey
Copy link
Member

amcasey commented Mar 4, 2020

@sandersn This was to support async file writing in build mode, which is currently on hold.

@rbuckton rbuckton marked this pull request as draft June 16, 2020 21:05
@sandersn
Copy link
Member

To help with PR housekeeping, I'm going to close this draft PR. It's pretty old.

@sandersn sandersn closed this May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants