fix: Add workaround for OpenTelemetry/Zone.js #1719
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| value: Value | Promise<Value> | ||
| ): value is Promise<Value> { | ||
| // https://github.com/amannn/next-intl/issues/1711 | ||
| return typeof (value as any).then === 'function'; |
There was a problem hiding this comment.
What do you think about adding an additional check of the well-known symbol [Symbol.toStringTag] prototype?
Zone.js returns 'Promise', the same as the native Promise. https://github.com/angular/zone.js/blob/master/lib/common/promise.ts#L364
Promise.prototype[Symbol.toStringTag] === 'Promise'
zonePromise[Symbol.toStringTag] === 'Promise'
There was a problem hiding this comment.
You mean in addition to then === 'function'? Hmm, it's an interesting point, I honestly didn't know about Symbol.toStringTag.
Based on the MDN docs on Symbol.toStringTag it seems like this would be safe to use on promises. However, I'm a bit reluctant as I've already encountered a range of weird cases where Next.js' overrides internals, patched something here and there, and sometimes things go off the rails.
Is there a specific use case where you think the current check might not work well?
As an additional note on the implementation, I put the focus on providing a minimal implementation that minifies as much as possible while being reliable. I took the is-promise implementation as inspiration and removed some checks that are not necessary for the ways isPromise is currently used in this codebase.
What do you think?
There was a problem hiding this comment.
Hi @amannn, first of all thanks for the quick support 😄, I tested locally with version 0.0.0-canary-84fe6d0 and it works as expected!
as @SalmenBejaoui suggest, probably Promise.prototype[Symbol.toStringTag] === 'Promise' could be useful, at least it should also handle the native promise override case.
about the zonePromise[Symbol.toStringTag] === 'Promise' could be a bit unsafe? it seems only zonejs related which isn't used on this library.
There was a problem hiding this comment.
Sure, that's great to hear!
What I'm trying to understand is how the additional usage of Symbol.toStringTag is benefical? Is there a case where the current check for then === 'function' is not sufficient?
Please also see:
My main motivation is to have something that minifies as much as possible. Additionally, for the way isPromise is used in this codebase we can take some shortcuts in comparison to the is-promise package npm.
There was a problem hiding this comment.
Oh ok, now with the Promise spec I got it, so yes, I think then === 'function' is sufficient to cover the promise case (I don't think that someone besides overwriting the global promise object, also edits the signature of the functions, hopefully...).
looks good like this to me.
There was a problem hiding this comment.
@amannn the thenable check is sufficient and follows A+ spec. Promise.prototype[Symbol.toStringTag] could be too strict.
I was thinking more widely, but it's not the case for next-intl.
isPromise({ then: () => {} }); // true
LGTM 👍
There was a problem hiding this comment.
Got it, sounds good! Many thanks for your research on this and for providing feedback, I really appreciate it and I'm sure other Next.js users will benefit from this too!
Fixes #1711