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
merged 2 commits into from Apr 3, 2019
Merged

Added Symbol.toStringTag support to Promise #1421

merged 2 commits into from Apr 3, 2019

Conversation

JoshuaKGoldberg
Copy link
Contributor

Fixes #1277.

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

@ThomWright
Copy link

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

@benjamingr
Copy link
Collaborator

@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
Copy link

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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

@cjbarth
Copy link

cjbarth 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
Copy link
Collaborator

, 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
Copy link
Contributor Author

Ping @petkaantonov, could you please merge this in?

@EToreo
Copy link

EToreo commented Dec 23, 2017

+1 to merge request!

@JFKingsley
Copy link

++

@peterjwest
Copy link

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

@toddbluhm
Copy link

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

peterjwest 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
Copy link
Collaborator

spion 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
Copy link

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
Copy link

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
Copy link
Collaborator

spion 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
Copy link

Awesome, thanks for the info!

@Xenya0815
Copy link

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
Copy link

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
Copy link
Collaborator

spion 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
Copy link
Contributor Author

JoshuaKGoldberg 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
Copy link
Collaborator

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
Copy link
Collaborator

spion 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

src/promise.js Outdated
if (typeof Symbol !== "undefined" && Symbol.toStringTag) {
es5.defineProperty(Promise.prototype, Symbol.toStringTag, {
get: function () {
return "Promise";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@spion spion Jun 13, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Symbol.toStringTag to promise instance