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.

Sign up for GitHub

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

fix: Add workaround for OpenTelemetry/Zone.js #1719

Merged
merged 4 commits into from
Feb 18, 2025
Merged

fix: Add workaround for OpenTelemetry/Zone.js #1719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b73586f
fix: Follow-up for #1573 to also handle the case when a non-default l…
amannn Nov 26, 2024
b44c8ef
Merge branch 'main' into canary
amannn Jan 21, 2025
b4de1a0
Merge remote-tracking branch 'origin/main' into canary
amannn Feb 17, 2025
84fe6d0
fix: Add workaround for OpenTelemetry/Zone.js (#1718)
amannn Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/next-intl/.size-limit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const config: SizeLimitConfig = [
{
name: "import * from 'next-intl' (react-server)",
path: 'dist/production/index.react-server.js',
limit: '14.735 KB'
limit: '15.355 KB'
},
{
name: "import {createSharedPathnamesNavigation} from 'next-intl/navigation' (react-client)",
Expand All @@ -21,13 +21,13 @@ const config: SizeLimitConfig = [
name: "import {createLocalizedPathnamesNavigation} from 'next-intl/navigation' (react-client)",
path: 'dist/production/navigation.react-client.js',
import: '{createLocalizedPathnamesNavigation}',
limit: '4.115 KB'
limit: '4.125 KB'
},
{
name: "import {createNavigation} from 'next-intl/navigation' (react-client)",
path: 'dist/production/navigation.react-client.js',
import: '{createNavigation}',
limit: '4.115 KB'
limit: '4.125 KB'
},
{
name: "import {createSharedPathnamesNavigation} from 'next-intl/navigation' (react-server)",
Expand Down Expand Up @@ -55,7 +55,7 @@ const config: SizeLimitConfig = [
{
name: "import * from 'next-intl/server' (react-server)",
path: 'dist/production/server.react-server.js',
limit: '14.035 KB'
limit: '14.635 KB'
},
{
name: "import createMiddleware from 'next-intl/middleware'",
Expand Down
4 changes: 3 additions & 1 deletion packages/next-intl/__mocks__/react.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import {isPromise} from '../src/shared/utils';

// @ts-expect-error -- React uses CJS
export * from 'react';

export {default} from 'react';

export function use(promise: Promise<unknown> & {value?: unknown}) {
if (!(promise instanceof Promise)) {
if (!isPromise(promise)) {
throw new Error('Expected a promise, got ' + typeof promise);
}

Expand Down
9 changes: 4 additions & 5 deletions packages/next-intl/src/navigation/shared/createSharedNavigationFns.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
Pathnames
} from '../../routing/types';
import {ParametersExceptFirst, Prettify} from '../../shared/types';
import {isLocalizableHref} from '../../shared/utils';
import {isLocalizableHref, isPromise} from '../../shared/utils';
import BaseLink from './BaseLink';
import {
HrefOrHrefWithParams,
Expand Down Expand Up @@ -111,10 +111,9 @@ export default function createSharedNavigationFns<
const isLocalizable = isLocalizableHref(href);

const localePromiseOrValue = getLocale();
const curLocale =
localePromiseOrValue instanceof Promise
? use(localePromiseOrValue)
: localePromiseOrValue;
const curLocale = isPromise(localePromiseOrValue)
? use(localePromiseOrValue)
: localePromiseOrValue;

const finalPathname = isLocalizable
? getPathname(
Expand Down
3 changes: 2 additions & 1 deletion packages/next-intl/src/react-server/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import {describe, expect, it, vi} from 'vitest';
import {getTranslations} from '../server.react-server';
import {isPromise} from '../shared/utils';
import {renderToStream} from './testUtils';
import {
_createCache,
Expand Down Expand Up @@ -73,7 +74,7 @@ describe('performance', () => {
try {
useTranslations('Component');
} catch (promiseOrError) {
if (promiseOrError instanceof Promise) {
if (isPromise(promiseOrError)) {
await promiseOrError;
useTranslations('Component');
} else {
Expand Down
5 changes: 2 additions & 3 deletions packages/next-intl/src/server/react-server/RequestLocale.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import {headers} from 'next/headers';
import {cache} from 'react';
import {HEADER_LOCALE_NAME} from '../../shared/constants';
import {isPromise} from '../../shared/utils';
import {getCachedRequestLocale} from './RequestLocaleCache';

async function getHeadersImpl(): Promise<Headers> {
const promiseOrValue = headers();

// Compatibility with Next.js <15
return promiseOrValue instanceof Promise
? await promiseOrValue
: promiseOrValue;
return isPromise(promiseOrValue) ? await promiseOrValue : promiseOrValue;
}
const getHeaders = cache(getHeadersImpl);

Expand Down
3 changes: 2 additions & 1 deletion packages/next-intl/src/server/react-server/getConfig.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
_createIntlFormatters,
initializeConfig
} from 'use-intl/core';
import {isPromise} from '../../shared/utils';
import {getRequestLocale} from './RequestLocale';
import {getRequestLocale as getRequestLocaleLegacy} from './RequestLocaleLegacy';
import createRequestConfig from './createRequestConfig';
Expand Down Expand Up @@ -72,7 +73,7 @@ See also: https://next-intl.dev/docs/usage/configuration#i18n-request
};

let result = getConfig(params);
if (result instanceof Promise) {
if (isPromise(result)) {
result = await result;
}

Expand Down
7 changes: 7 additions & 0 deletions packages/next-intl/src/shared/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,3 +233,10 @@ function comparePathnamePairs(a: string, b: string): number {
export function getSortedPathnames(pathnames: Array<string>) {
return pathnames.sort(comparePathnamePairs);
}

export function isPromise<Value>(
value: Value | Promise<Value>
): value is Promise<Value> {
// https://github.com/amannn/next-intl/issues/1711
return typeof (value as any).then === 'function';
Copy link
Contributor

Choose a reason for hiding this comment

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

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'

Copy link
Owner Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@SalmenBejaoui SalmenBejaoui Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

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!

}
Loading