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

Allow numberFormat to degrade gracefully. #3620

Merged
merged 18 commits into from
Jun 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 48 additions & 1 deletion assets/js/util/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* External dependencies
*/
import { get, isFinite, isPlainObject } from 'lodash';
import memize from 'memize';

/**
* WordPress dependencies
Expand Down Expand Up @@ -272,6 +273,9 @@ export const numFmt = ( number, options = {} ) => {
return numberFormat( number, formatOptions );
};

// Warn once for a given message.
const warnOnce = memize( console.warn ); // eslint-disable-line no-console

/**
* Formats a number using the JS Internationalization Number Format API.
*
Expand All @@ -286,7 +290,50 @@ export const numFmt = ( number, options = {} ) => {
export const numberFormat = ( number, options = {} ) => {
const { locale = getLocale(), ...formatOptions } = options;

return new Intl.NumberFormat( locale, formatOptions ).format( number );
try {
/**
* Per https://github.com/google/site-kit-wp/issues/3255 there have been issues with some versions of Safari
* on some operating systems throwing issues with some parameters in the formatOptions.
*
* If an error is thrown, we remove some troublesome params from the formatOptions object and fallback to no formatting.
*
* This allows us to degrade somewhat gracefully without breaking the dashboard for users of unaffected browsers.
*/
return new Intl.NumberFormat( locale, formatOptions ).format( number );
} catch ( error ) {
warnOnce( `Site Kit numberFormat error: Intl.NumberFormat( ${ JSON.stringify( locale ) }, ${ JSON.stringify( formatOptions ) } ).format( ${ typeof number } )`, error.message );
}

// Remove these key/values from formatOptions.
const unstableFormatOptionValues = {
currencyDisplay: 'narrow',
currencySign: 'accounting',
style: 'unit',
};

// Remove these keys from formatOptions irrespective of value.
const unstableFormatOptions = [
'signDisplay',
'compactDisplay',
];

const reducedFormatOptions = {};

for ( const [ key, value ] of Object.entries( formatOptions ) ) {
if ( unstableFormatOptionValues[ key ] && value === unstableFormatOptionValues[ key ] ) {
continue;
}
if ( unstableFormatOptions.includes( key ) ) {
continue;
}
reducedFormatOptions[ key ] = value;
}

try {
return new Intl.NumberFormat( locale, reducedFormatOptions ).format( number );
} catch {
return new Intl.NumberFormat( locale ).format( number );
}
};

/**
Expand Down
95 changes: 95 additions & 0 deletions assets/js/util/test/numberFormat.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ describe( 'numberFormat', () => {
expect(
numberFormat( 123456789.87, { locale: 'de-DE' } )
).toStrictEqual( '123.456.789,87' );
expect( console ).not.toHaveWarned();
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
} );

afterEach( () => {
Expand Down Expand Up @@ -115,5 +116,99 @@ describe( 'numberFormat', () => {
it.each( siteKitLocales )( 'formats numbers correctly with locale variant %s', ( locale, value, expected ) => {
setupGoogleSiteKit( locale );
expect( numberFormat( value ) ).toStrictEqual( expected );
expect( console ).not.toHaveWarned();
} );

describe( 'graceful degradation for problematic options in some browsers', () => {
const NumberFormat = Intl.NumberFormat;
let NumberFormatSpy;

beforeEach( () => {
NumberFormatSpy = jest.spyOn( global.Intl, 'NumberFormat' );
} );

afterEach( () => {
NumberFormatSpy.mockRestore();
} );

// Error message that browser throws on error.
const errorMessage = 'TypeError: Failed to initialize NumberFormat since used feature is not supported in the linked ICU version';

// Replicate a browser behaviour to throw errors when certain option key/values are encountered.
const createThrowIfOptionMatch = ( key, value ) => ( locales, options = {} ) => {
if ( options[ key ] && (
value === options[ key ] || value === undefined
) ) {
throw new TypeError( errorMessage );
}
return NumberFormat( locales, options );
};

it( 'degrades gracefully when `signDisplay` has any value other than the default of `auto`', () => {
// Regular implementation.
expect( numberFormat( -0.0123, {
locale: 'en-US',
signDisplay: 'never',
style: 'percent',
maximumFractionDigits: 1,
} ) ).toStrictEqual( '1.2%' );

/*
* Option of `signDisplay: never` causes issues in some browser/os combinations.
*
* @see https://github.com/google/site-kit-wp/issues/3255
*/
NumberFormatSpy.mockImplementation( createThrowIfOptionMatch( 'signDisplay', 'never' ) );

expect( numberFormat( -0.0123, {
locale: 'en-US',
signDisplay: 'never', // This parameter will be removed.
style: 'percent',
maximumFractionDigits: 1,
} ) ).toStrictEqual( '-1.2%' );

const expectedWarning = 'Site Kit numberFormat error: Intl.NumberFormat( "en-US", {"signDisplay":"never","style":"percent","maximumFractionDigits":1} ).format( number )';
expect( console ).toHaveWarnedWith( expectedWarning, errorMessage );

// Call the same function again to ensure we don't warn again.
numberFormat( -0.0123, {
locale: 'en-US',
signDisplay: 'never', // This parameter will be removed.
style: 'percent',
maximumFractionDigits: 1,
} );

// Ensure we don't log more than once.
expect( console.warn ).toHaveBeenCalledTimes( 1 ); // eslint-disable-line no-console
} );

it( 'degrades gracefully when the `style:unit` option is provided', () => {
// Regular implementation.
expect( numberFormat( 22, {
locale: 'en-US',
unitDisplay: 'narrow',
style: 'unit',
unit: 'second',
} ) ).toStrictEqual( '22s' );

expect( console ).not.toHaveWarned();

/*
* Option of `style: unit` causes issues in some browser/os combinations.
*
* @see https://github.com/google/site-kit-wp/issues/3255
*/
NumberFormatSpy.mockImplementation( createThrowIfOptionMatch( 'style', 'unit' ) );

expect( numberFormat( 22, {
locale: 'en-US',
unitDisplay: 'narrow',
style: 'unit',
unit: 'second',
} ) ).toStrictEqual( '22' );

const expectedWarning = 'Site Kit numberFormat error: Intl.NumberFormat( "en-US", {"unitDisplay":"narrow","style":"unit","unit":"second"} ).format( number )';
expect( console ).toHaveWarnedWith( expectedWarning, errorMessage );
} );
} );
} );