Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

fix: Use @guardian/tsconfig #390

Merged
merged 10 commits into from
Aug 24, 2022
Merged

fix: Use @guardian/tsconfig #390

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1e9c9fb
fix: use `@guardian/tsconfig`
sndrs Aug 23, 2022
1a504be
Use `Date.toLocaleString` instead of an array of month names
sndrs Aug 23, 2022
861093a
shorten the logic checks
sndrs Aug 23, 2022
af1e46f
fix that broken logic
sndrs Aug 23, 2022
15f7353
Update src/cookies/getCookie.ts
sndrs Aug 23, 2022
5dc53fe
Update src/cookies/setCookie.ts
sndrs Aug 23, 2022
4b8f951
Update src/cookies/setSessionCookie.ts
sndrs Aug 23, 2022
f903765
remove uneeded config
sndrs Aug 23, 2022
db2285b
also uneeded
sndrs Aug 23, 2022
e7ea101
_also_ unneeded!
sndrs Aug 23, 2022
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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"@guardian/eslint-config-typescript": "1.0.2",
"@guardian/pkgu": "0.6.2",
"@guardian/prettier": "2.0.0",
"@guardian/tsconfig": "0.1.4",
"@semantic-release/github": "8.0.5",
"@types/jest": "27.4.1",
"@types/node": "16.11.47",
Expand Down
2 changes: 1 addition & 1 deletion src/cookies/getCookie.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const getCookie = ({

const cookieVal = getCookieValues(name);

if (cookieVal.length > 0) {
if (cookieVal[0]) {
sndrs marked this conversation as resolved.
Show resolved Hide resolved
if (shouldMemoize) {
memoizedCookies.set(name, cookieVal[0]);
}
Expand Down
5 changes: 4 additions & 1 deletion src/cookies/setCookie.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ export const setCookie = ({

// If the cookie is already memoized we want to replace its value
if (memoizedCookies.has(name)) {
memoizedCookies.set(name, getCookieValues(name)[0]);
const value = getCookieValues(name)[0];
sndrs marked this conversation as resolved.
Show resolved Hide resolved
if (value) {
memoizedCookies.set(name, value);
}
}
};
5 changes: 4 additions & 1 deletion src/cookies/setSessionCookie.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ export const setSessionCookie = ({

// If the cookie is already memoized we want to replace its value
if (memoizedCookies.has(name)) {
memoizedCookies.set(name, getCookieValues(name)[0]);
const value = getCookieValues(name)[0];
sndrs marked this conversation as resolved.
Show resolved Hide resolved
if (value) {
memoizedCookies.set(name, value);
}
}
};
36 changes: 3 additions & 33 deletions src/datetime/timeAgo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,5 @@
type Unit = 's' | 'm' | 'h' | 'd';

const shortMonth = (month: number): string =>
[
'Jan',
'Feb',
'Mar',
'Apr',
'May',
'Jun',
'Jul',
'Aug',
'Sep',
'Oct',
'Nov',
'Dec',
][month];

const longMonth = (month: number): string =>
[
'January',
'February',
'March',
'April',
'May',
'June',
'July',
'August',
'September',
'October',
'November',
'December',
][month];

const pad = (n: number): number | string => n.toString().padStart(2, '0');

const isWithin24Hours = (date: Date): boolean => {
Expand Down Expand Up @@ -137,7 +105,9 @@ export const timeAgo = (
// Simple date - "9 Nov 2019"
return [
then.getDate(),
verbose ? longMonth(then.getMonth()) : shortMonth(then.getMonth()),
verbose
Copy link

Choose a reason for hiding this comment

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

ah! a lot nicer :)

? then.toLocaleString('default', { month: 'long' })
: then.toLocaleString('default', { month: 'short' }),
Comment on lines +109 to +110
Copy link
Member Author

@sndrs sndrs Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@oliverlloyd how do you feel about these? guardian is at 98.99% support (greater than es module support). are they going to have wierd unexpected effects?

Copy link
Contributor

@JamieB-gu JamieB-gu Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Given that the other functions in this file return English strings, and the style guide is British English, is it possible we want to use 'en-GB' here?

Suggested change
? then.toLocaleString('default', { month: 'long' })
: then.toLocaleString('default', { month: 'short' }),
? then.toLocaleString('en-GB', { month: 'long' })
: then.toLocaleString('en-GB', { month: 'short' }),

I think using 'default' will use the browser/system's default locale, and will therefore give the month in the language for that locale? For example, in French if the system is set to French. You can test this with the following snippet of code:

const d = new Date();

d.toLocaleString('default', { month: 'long' });

in Node by setting the LANG environment variable:

LANG=fr_FR node

or in a browser by changing the macOS system language to e.g. French, restarting your browser, and then pulling up a console (tested in Firefox and Safari).

Copy link
Contributor

@mxdvl mxdvl Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Completely agree with @JamieB-gu here! I would be tempted to move this to a separate PR, as it’s possibly a breaking change, or at least most definitly a feature.

I am one such user who would not want my dates in French if I’m browsing an english-language website.

My suggestion for the existing code would be:

const longMonth = (month: number): string =>
	([
		'January',
		'February',
		'March',
		'April',
		'May',
		'June',
		'July',
		'August',
		'September',
		'October',
		'November',
		'December',
	] as const)[month] ?? "Unknown";
	
const shortMonth = (month: number) => longMonth(month).slice(0,3);

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a great point – 'en-GB' should resolve that though?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think then.toLocaleString('en-GB', { month: 'short' }) best respects the design guide

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that toLocaleString was not well supported in browsers, but it seems like that’s not the case, there’s only missing data for about 3% of browsers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Support is available for everything that supports es modules afaict?

then.getFullYear(),
].join(' ');
}
Expand Down
6 changes: 3 additions & 3 deletions src/loadScript/loadScript.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ describe('loadScript', () => {
referrerPolicy: 'no-referrer',
className: 'u6ytfiuyoibnpoim',
});
expect(document.scripts[0].async).toBeTruthy();
expect(document.scripts[0].referrerPolicy).toBe('no-referrer');
expect(document.scripts[0].className).toBe('u6ytfiuyoibnpoim');
expect(document.scripts[0]?.async).toBeTruthy();
expect(document.scripts[0]?.referrerPolicy).toBe('no-referrer');
expect(document.scripts[0]?.className).toBe('u6ytfiuyoibnpoim');
});

it('rejects if the script fails to load', async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/loadScript/loadScript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,5 @@ export const loadScript = (
script.onerror = reject;

const ref = document.scripts[0];
ref.parentNode?.insertBefore(script, ref);
ref?.parentNode?.insertBefore(script, ref);
});
33 changes: 17 additions & 16 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
{
"compilerOptions": {
"allowJs": true,
"allowSyntheticDefaultImports": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"isolatedModules": true,
"module": "ESNext",
"moduleResolution": "node",
"noEmit": true,
"resolveJsonModule": true,
"strict": true,
"target": "ES2020"
},
"include": ["src"],
"exclude": ["node_modules", "dist", "coverage"],
"ts-node": { "compilerOptions": { "module": "commonjs" } }
"extends": "@guardian/tsconfig",
"compilerOptions": {
"allowJs": true,
"allowSyntheticDefaultImports": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"isolatedModules": true,
"module": "ESNext",
sndrs marked this conversation as resolved.
Show resolved Hide resolved
"moduleResolution": "node",
"noEmit": true,
"resolveJsonModule": true,
"strict": true,
"target": "ES2020"
},
"include": ["src"],
"exclude": ["node_modules", "dist", "coverage"],
"ts-node": { "compilerOptions": { "module": "commonjs" } }
}
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,11 @@
resolved "https://registry.yarnpkg.com/@guardian/prettier/-/prettier-2.0.0.tgz#2d213e78fab5973285ef5e4f077d3439ec661f49"
integrity sha512-xzihXsXQGERmbl6PSfC17o8h/R8SVO2DVHY7npxzSdKhQdGSRivWvIKOOsroEBoO5RsBMqeo2NJ+VSWQtugViw==

"@guardian/[email protected]":
version "0.1.4"
resolved "https://registry.npmjs.org/@guardian/tsconfig/-/tsconfig-0.1.4.tgz#6f9b78f36b14dbe9507658e7d8bb49b61398b641"
integrity sha512-QLHYlDSUksxQw3hzbAFhdC/NS7RMlw7fsdg0fL2CaOeGmOSwEmPJTbsqJAp3KxvpCuLK3ZtCSDnOY7sLcuQ+Ig==

"@humanwhocodes/config-array@^0.10.4":
version "0.10.4"
resolved "https://registry.yarnpkg.com/@humanwhocodes/config-array/-/config-array-0.10.4.tgz#01e7366e57d2ad104feea63e72248f22015c520c"
Expand Down