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

Added Symbol.toStringTag support to Promise #1421

Merged

Conversation

Projects
None yet
@JoshuaKGoldberg
Copy link
Contributor

commented Jul 13, 2017

Fixes #1277.

Doesn't come with tests (yet?) - would you like me to add them, and if so where?

@ThomWright

This comment has been minimized.

Copy link

commented Oct 30, 2017

Any chance of getting this merged? This is a real pain when using bluebird in Typescript.

@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Oct 30, 2017

@petkaantonov? I'm +0 on this since it's a used and known trick for detecting non-native promises vs. native ones. Implementation looks correct.

@ThomWright

This comment has been minimized.

Copy link

commented Oct 30, 2017

This implementation returns "Promise" - is there a reason why it must do so? Could we return "BluebirdPromise" or similar? That way existing clients using that trick might still work (as long as they check the result).

@peterjwest

This comment has been minimized.

Copy link

commented Nov 7, 2017

Simply "Bluebird" might make more sense, since this is the bluebird constructor name.

Is there any way I can help with this? (Also keen to fix typescript issues).

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2017

return "BluebirdPromise" or similar
"Bluebird" might make more sense

I suspect that's not a good idea. Promises return "[object Promise]" for both promise.toString() and Object.prototype.toString.call(promise). Bluebird already returns the same for .toString. It would be weird to return something different for toString.call.

const Bluebird = require("bluebird");

const bluebird = new Bluebird(() => {});
const native = new Promise(() => {});

console.log("bluebirdtoString", bluebird.toString());
console.log("native toString", native.toString());

console.log("bluebird O.call", Object.prototype.toString.call(bluebird));
console.log("native O.call", Object.prototype.toString.call(native));

Two other examples of why giving something different might be funky:

  • Libraries might check the toString of objects to see if they're Promises (and not using instanceof Promise to account for polyfills like Bluebird)
  • TypeScript/Flow typings allow for specifying string literals. If the Promise type declares that Symbol.toStringTag should give "Promise", users would get conflicting type errors trying to use Bluebird with code that expects Promise-likes (including all async/await code).

Still, I haven't researched this much beyond those suspicions. It'd be great if someone proved me wrong - I'm up for updating this PR if so.

@peterjwest

This comment has been minimized.

Copy link

commented Nov 10, 2017

You're right about TypeScript - the type is specified as a literal, and your reasoning makes sense.

@cjbarth

This comment has been minimized.

Copy link

commented Nov 11, 2017

If the only objection to this PR is that is makes it harder for users to detect if the promise library is Bluebird or Native, then why don't we just include a property on the Bluebird object called version or bluebird that will only exist for Bluebird promises and let people check for that instead? Otherwise, if Bluebird is 100% compatible with Native promises except for this function, then why would users care so much which promise library is servicing the promise they are working with?

@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Nov 11, 2017

, then why don't we just include a property on the Bluebird object called version or bluebird that will only exist for Bluebird promises and let people check for that instead?

Someone might be checking for native promises by checking the object toString name, with this they'd have to always consider bluebird.

if Bluebird is 100% compatible with Native promises except for this function, then why would users care so much which promise library is servicing the promise they are working with?

For example, if it is not a native promise they might need to hook it differently for instrumentation, coerce it or behave differently (for example if they're adding properties on it which might collide).

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

Ping @petkaantonov, could you please merge this in?

@EToreo

This comment has been minimized.

Copy link

commented Dec 23, 2017

+1 to merge request!

@JFKingsley

This comment has been minimized.

Copy link

commented Jan 11, 2018

++

@peterjwest

This comment has been minimized.

Copy link

commented Jan 11, 2018

@benjamingr would you mind looking at this? I think it is fit to be merged.

@toddbluhm

This comment has been minimized.

Copy link

commented Jan 17, 2018

Would really appreciate this getting merged in some time. I feel like bluebird is a real pain to use in typescript because of this compatibility issue.

@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2018

Would really appreciate this getting merged in some time. I feel like bluebird is a real pain to use in typescript because of this compatibility issue.

wait, what?

@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2018

I use bluebird with TS all the time and I haven't noticed Symbol.toStringTag giving me problems (like, ever). This was a design decision to not implement initially - and I'm waiting for @petkaantonov to get on a development spree to merge/release since I didn't think it's a bug but rather a subjective change

@peterjwest

This comment has been minimized.

Copy link

commented Jan 18, 2018

Here's a minimal example:

const x = new Bluebird(() => { return; });

function takePromise<T>(a: Promise<T>) {
  return a;
}

takePromise(x);

This fails to compile with the error:

[ts]
Argument of type 'Bluebird<{}>' is not assignable to parameter of type 'Promise<{}>'.
  Property '[Symbol.toStringTag]' is missing in type 'Bluebird<{}>'.

Bluebird promises are not currently compatible with the Promise interface, so any third party library that returns a non-bluebird promise will have to be wrapped in a bluebird promise, and bluebird promise types have to be used everywhere.

How is it a subjective change to expect a promise library to be compatible with the interface of a promise?

@spion

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2018

Casting an external promise type to bluebird would still be necessary to use any bluebird specific methods.

For taking promise arguments you can use PromiseLike<T> instead of Promise<T>

A second option is to use a definition file that simply claims that Symbol.toStringTag is present in the bluebird class (even though its not). Depending on the circumstances, this could be an acceptable "lie".

This might be an acceptable breaking change, but might also necessitate a major version bump. Its difficult to say if the compatibility breakage is too bad.

We might be able to get away with redefining toString() to keep returning the same thing as before - that way there would be no compatibility issue, however there will be inconsistency between the string tag value and toString(). That could be an acceptable compromise (and we could postpone the toString value update for v4)

@peterjwest

This comment has been minimized.

Copy link

commented Jan 19, 2018

That makes sense to me.

Re: PromiseLIke. I have had issues with using it in place of Promise with Bluebird:

function returnPromiseLike(): PromiseLike<string> {
  return new Promise<string>((resolve) => resolve('foo'));
}

function returnPromiseLikeBluebird(): PromiseLike<string> {
  return returnPromiseLike()
  .then((foo: string) => {
    return new Bluebird<string>((resolve) => resolve('bar'));
  });
}
[ts]
Type 'PromiseLike<Bluebird<string>>' is not assignable to type 'PromiseLike<string>'.
  Type 'Bluebird<string>' is not assignable to type 'string'.

I don't know enough about the types to debug this. My life using Bluebird + Typescript has been constant juggling of types between Bluebird, Promise and PromiseLike. 😞

@zemlanin

This comment has been minimized.

Copy link

commented Jan 19, 2018

I got this problem when I've tried to use await keyword in combination with sequelize's database queries. Workaround is somewhat risky but fine for my cases

interface ESPromise<T> extends Promise<T> {}

declare module "sequelize" {
  interface Promise<R> extends ESPromise<R> {
    [Symbol.toStringTag]: "Promise";
  }
}

@peterjwest I hope this can help you

@spion

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2018

@peterjwest that actually seems to be a bug with the TypeScript compiler. Try copying the exact same interface as PromiseLike<T> from the .d.ts, rename it PromiseLikeFixed<T>, and use PromiseLikeFixed<T> instead in your example. It will suddenly typecheck:

import * as Bluebird from 'bluebird'

interface PromiseLikeFixed<T> {
  /**
   * Attaches callbacks for the resolution and/or rejection of the Promise.
   * @param onfulfilled The callback to execute when the Promise is resolved.
   * @param onrejected The callback to execute when the Promise is rejected.
   * @returns A Promise for the completion of which ever callback is executed.
   */
  then<TResult1 = T, TResult2 = never>(
    onfulfilled?: ((value: T) => PromiseLike<TResult1> | TResult1) | undefined | null, 
    onrejected?: ((reason: any) => PromiseLike<TResult2> | TResult2) | undefined | null): 
    PromiseLike<TResult1 | TResult2>;
}

function returnPromiseLike(): PromiseLikeFixed<string> {
  return new Promise<string>((resolve) => resolve('foo'));
}


function returnBB() {
  return Bluebird.resolve('s')
}
function returnPromiseLikeBluebird(): PromiseLikeFixed<string> {
  return returnPromiseLike()
    .then((_foo: string) => returnBB())
}

Seems to have already been reported here: microsoft/TypeScript#17862

@peterjwest

This comment has been minimized.

Copy link

commented Jan 19, 2018

Awesome, thanks for the info!

@Xenya0815

This comment has been minimized.

Copy link

commented May 9, 2018

Is it planed to merge that PR? I would like to see that! The suggested solution, to use PromiseLike instead of Promise, is not possible for me because I don't want to hide the catch method.

@DavidMorton

This comment has been minimized.

Copy link

commented May 9, 2018

I'll throw in my solution for the Typescript issue.

  1. Create a local copy of bluebird.d.ts.

  2. Add the following lines somewhere in the class definition for Bluebird.
    [Symbol.toStringTag];
    valueOf (): symbol;

  3. Insert a path into the tsconfig.json to point to this new file. I have the d.ts file at the same level as the tsconfig.json in my project, so mine looks like this (simplified):
    "paths": {
    "bluebird": "bluebird.d.ts"
    }

  4. At the top of every entry file (not all files, but just the starting points for various processes/forks/etc for your application, add the following:
    insert * as Promise from 'bluebird';

I haven't had any adverse effects doing this, and everything, including async/await (even combined with try/catch/finally) seems to work just fine now.

Sorry, just couldn't wait forever for this pull request to be completed. I got work to do.

@spion

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2018

@JoshuaKGoldberg if you are still interested in this, I have two questions

  • can we change it so that Object.prototype.toString.call(promise) returns the same thing it did before
  • can we do some research whether any other library is using the presence of [Symbol.toStringTag] directly to tell native from BB promises apart.
@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2018

Still interested!

Object.prototype.toString.call(promise)

I don't think we can change the behavior of that. Some sample outputs in Node:

> Object.prototype.toString.call({ toString: () => "hi" })
'[object Object]'

> Object.prototype.toString.call(undefined)
'[object Undefined]'

> Object.prototype.toString.call("")
'[object String]'

> Object.prototype.toString.call(new (class Promise { }))
'[object Object]'

> Object.prototype.toString.call(new Promise(() => { }))
'[object Promise]'

Edit: Oh, I see what you're saying. Yes, this gives it the same [object Promise] as before.

class MyPromise { }
Object.defineProperty(MyPromise.prototype, Symbol.toStringTag, { get: () => "Promise" });
(new MyPromise()).toString()

// '[object Promise]'

using the presence of [Symbol.toStringTag] directly to tell native from BB promises apart.

I haven't seen such a thing. Perhaps someone else on this thread has?

@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2018

I haven't seen such a thing. Perhaps someone else on this thread has?

I have. Which is why I don't understand why this is a bluebird issue rather than something bluebird.d.ts can fix.

@spion

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2018

@benjamingr Its because [Symbol.toStringTag] does not exist - adding it to bluebird.d.ts would be lying to the compiler.

For the one you've seen, do they test for existance of the toStringTag as a property directly or are they comparing it to a value? If they're comparing it to a value, we can simply return a different value - one that would not change the current result of Object.prototype.toString.call(bluebirdPromise) - i.e. we can make it "Object"

The main concern are libraries and most of those are open source. I don't think end-user apps would be a huge issue. We always have the option to bump to bluebird 4.0

if (typeof Symbol !== "undefined" && Symbol.toStringTag) {
es5.defineProperty(Promise.prototype, Symbol.toStringTag, {
get: function () {
return "Promise";

This comment has been minimized.

Copy link
@spion

spion Jun 13, 2018

Collaborator

If the value returned is "Object" instead, we would get the exact same external toString.call behavior as we do now

This comment has been minimized.

Copy link
@spion

spion Jun 13, 2018

Collaborator

Additionally, libraries that use x[Symbol.toStringTag] === 'Promise' to differentiate native promises from Bluebird will continue to work normally. Only libraries that check for the mere presence of x[Symbol.toStringTag] will have problems.

Hmm. I see we'd still have an incompatible type though. Not sure what to do about this.

It looks like TS may have made an error here and effectively introduced a silly nominal type system.

This comment has been minimized.

Copy link
@spion
@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2018

@spion I defer to you - feel free to land and do a release.

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2018

Per @spion's comments and the thread in microsoft/TypeScript#19006 (comment), it looks like TypeScript should move away from typing it to "Promise" and instead have it as a general string. Changed this PR to return "Object" for Symbol.toStringTag.

@wallzero

This comment has been minimized.

Copy link

commented Sep 12, 2018

...can we lie to the compiler while we wait for TypeScript to fix this?

@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2018

...can we lie to the compiler while we wait for TypeScript to fix this?

Sure, you can extend the interface bluebird.d.ts exposes and use interface merging to add a toStringTag.

@spion

This comment has been minimized.

Copy link
Collaborator

commented Nov 10, 2018

Looks like TypeScript just merged a PR that relaxes the types microsoft/TypeScript#24396 . I think we can safely merge this now.

@josephgardner

This comment has been minimized.

Copy link

commented Dec 15, 2018

Any update on this?

@tmueller

This comment has been minimized.

Copy link

commented Jan 17, 2019

With Typescript 3.2. released, can this be merged?

@spion

This comment has been minimized.

Copy link
Collaborator

commented Jan 17, 2019

@petkaantonov @benjamingr i think this should be ok to merge provided we make a release

@superhawk610

This comment has been minimized.

Copy link

commented Mar 9, 2019

Any updates on this?

@PikachuEXE

This comment has been minimized.

Copy link

commented Mar 29, 2019

Any update on this?
Sorry nothing valuable is added
But I am using TS 3.4 already and feel bad seeing this still not merged

@benjamingr benjamingr merged commit 17f69f3 into petkaantonov:master Apr 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamingr

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

Thanks, yeah this should land, @petkaantonov will release.

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Haha I'd given up hope on this PR... Thanks everyone! 🙌

@JoshuaKGoldberg JoshuaKGoldberg deleted the JoshuaKGoldberg:symbol-to-string-tag branch Apr 3, 2019

@lhecker

This comment has been minimized.

Copy link

commented Apr 17, 2019

@petkaantonov This PR didn't land as part of the 3.5.4 release. Do you intend to release these changes soon?
And if so: Will this be a patch or a minor release?

@lhecker

This comment has been minimized.

Copy link

commented Apr 19, 2019

@benjamingr Do you (alternatively) happen to know when this feature might be released?

@petkaantonov

This comment has been minimized.

Copy link
Owner

commented May 27, 2019

released in 3.5.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.