fix: Use `@guardian/tsconfig` #390

sndrs · 2022-08-23T09:17:37Z

What does this change?

Adds @guardian/tsconfig to the typscript config

Why?

It's stricter (in a good way) and preps the code for import to https://github.com/guardian/csnx

github-actions · 2022-08-23T09:17:52Z

`yarn.lock` changes

Click to toggle table visibility

Name	Status	Previous	Current
`@guardian/tsconfig`		-	0.1.4

coveralls · 2022-08-23T09:18:47Z

Coverage remained the same at 100.0% when pulling e7ea101 on use-guardian-tsconfig into d5953f4 on main.

ob6160

nice work 👍

src/cookies/getCookie.ts

ob6160 · 2022-08-23T09:32:45Z

src/datetime/timeAgo.ts

@@ -137,7 +105,9 @@ export const timeAgo = (
 		// Simple date - "9 Nov 2019"
 		return [
 			then.getDate(),
-			verbose ? longMonth(then.getMonth()) : shortMonth(then.getMonth()),
+			verbose


ah! a lot nicer :)

sndrs · 2022-08-23T09:33:36Z

src/datetime/timeAgo.ts

+				? then.toLocaleString('default', { month: 'long' })
+				: then.toLocaleString('default', { month: 'short' }),


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

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).

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);

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

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

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.

Support is available for everything that supports es modules afaict?

have a ☕ first

mxdvl

This is great, but I would not change the behaviour of timeAgo as part of this change.

src/cookies/getCookie.ts

src/cookies/setCookie.ts

src/cookies/setSessionCookie.ts

mxdvl · 2022-08-23T11:52:32Z

src/datetime/timeAgo.ts

+				? then.toLocaleString('default', { month: 'long' })
+				: then.toLocaleString('default', { month: 'short' }),


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);

tsconfig.json

I don’t think this should go in as-is, because of the timeAgo changes.

Co-authored-by: Max Duval <[email protected]>

sndrs · 2022-08-23T12:39:46Z

I don’t think this should go in as-is, because of the timeAgo changes.

what do you think needs to change to make them ok? the ?? "Unknown" is only there because as far as i know TS doesn't have a type for 0-11. that makes it a branch of code we can't test, and given the month index comes from getMonth, a branch we'll never use. it's only there because getMonth returns a number in ts.

if the en-GB @JamieB-gu mentioned returns the same strings, shouldn't we be ok? what would you think we'd need to address in a subsequent PR?

mxdvl

Indeed, with the great support of toLocaleString, I think this is good to go!

mxdvl · 2022-08-23T14:43:26Z

src/datetime/timeAgo.ts

+				? then.toLocaleString('default', { month: 'long' })
+				: then.toLocaleString('default', { month: 'short' }),


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.

github-actions · 2022-08-24T09:37:01Z

🎉 This PR is included in version 7.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

JamieB-gu · 2022-08-24T16:57:06Z

@sndrs did we want to change the toLocaleString call to specify en-GB? I think, with this merged, the code now on main will show datetimes in different languages if any of the consumers update to it? I may have missed something though 🤔

sndrs · 2022-08-24T17:27:52Z

Doh! Yes I completely forgot to implement it!

was meant to happen in #390

sndrs · 2022-08-24T17:33:59Z

@JamieB-gu correct locale went out in https://github.com/guardian/libs/releases/tag/v7.1.3

sndrs requested a review from a team as a code owner August 23, 2022 09:17

sndrs enabled auto-merge (squash) August 23, 2022 09:17

fix: use @guardian/tsconfig

1e9c9fb

sndrs force-pushed the use-guardian-tsconfig branch from 1a95c61 to 1e9c9fb Compare August 23, 2022 09:18

ob6160 previously approved these changes Aug 23, 2022

View reviewed changes

Use Date.toLocaleString instead of an array of month names

1a504be

sndrs dismissed ob6160’s stale review via 1a504be August 23, 2022 09:30

shorten the logic checks

861093a

ob6160 reviewed Aug 23, 2022

View reviewed changes

sndrs commented Aug 23, 2022

View reviewed changes

fix that broken logic

af1e46f

have a ☕ first

sndrs requested review from oliverlloyd and mxdvl August 23, 2022 09:38

sndrs disabled auto-merge August 23, 2022 09:39

mxdvl previously approved these changes Aug 23, 2022

View reviewed changes

sndrs and others added 4 commits August 23, 2022 13:24

Update src/cookies/getCookie.ts

15f7353

Co-authored-by: Max Duval <[email protected]>

Update src/cookies/setCookie.ts

5dc53fe

Co-authored-by: Max Duval <[email protected]>

Update src/cookies/setSessionCookie.ts

4b8f951

Co-authored-by: Max Duval <[email protected]>

remove uneeded config

f903765

sndrs added 2 commits August 23, 2022 13:41

also uneeded

db2285b

_also_ unneeded!

e7ea101

mxdvl approved these changes Aug 23, 2022

View reviewed changes

sndrs merged commit c5b6652 into main Aug 24, 2022

sndrs deleted the use-guardian-tsconfig branch August 24, 2022 09:34

github-actions bot added the released label Aug 24, 2022

sndrs added a commit that referenced this pull request Aug 24, 2022

fix: ensure timeAgo uses uk month names

9e613cf

was meant to happen in #390

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use `@guardian/tsconfig` #390

fix: Use `@guardian/tsconfig` #390

sndrs commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

coveralls commented Aug 23, 2022 •

edited

Loading

ob6160 left a comment

ob6160 Aug 23, 2022

sndrs Aug 23, 2022 •

edited

Loading

JamieB-gu Aug 23, 2022 •

edited

Loading

mxdvl Aug 23, 2022 •

edited

Loading

sndrs Aug 23, 2022

oliverlloyd Aug 23, 2022

mxdvl Aug 23, 2022

sndrs Aug 24, 2022

mxdvl left a comment

mxdvl Aug 23, 2022 •

edited

Loading

sndrs commented Aug 23, 2022

mxdvl left a comment

mxdvl Aug 23, 2022

github-actions bot commented Aug 24, 2022

JamieB-gu commented Aug 24, 2022 •

edited

Loading

sndrs commented Aug 24, 2022

sndrs commented Aug 24, 2022

		? then.toLocaleString('default', { month: 'long' })
		: then.toLocaleString('default', { month: 'short' }),

fix: Use @guardian/tsconfig #390

fix: Use @guardian/tsconfig #390

Conversation

sndrs commented Aug 23, 2022

What does this change?

Why?

github-actions bot commented Aug 23, 2022

yarn.lock changes

coveralls commented Aug 23, 2022 • edited Loading

ob6160 left a comment

Choose a reason for hiding this comment

ob6160 Aug 23, 2022

Choose a reason for hiding this comment

sndrs Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

JamieB-gu Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

mxdvl Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

sndrs Aug 23, 2022

Choose a reason for hiding this comment

oliverlloyd Aug 23, 2022

Choose a reason for hiding this comment

mxdvl Aug 23, 2022

Choose a reason for hiding this comment

sndrs Aug 24, 2022

Choose a reason for hiding this comment

mxdvl left a comment

Choose a reason for hiding this comment

mxdvl Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

sndrs commented Aug 23, 2022

mxdvl left a comment

Choose a reason for hiding this comment

mxdvl Aug 23, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022

JamieB-gu commented Aug 24, 2022 • edited Loading

sndrs commented Aug 24, 2022

sndrs commented Aug 24, 2022

fix: Use `@guardian/tsconfig` #390

fix: Use `@guardian/tsconfig` #390

`yarn.lock` changes

coveralls commented Aug 23, 2022 •

edited

Loading

sndrs Aug 23, 2022 •

edited

Loading

JamieB-gu Aug 23, 2022 •

edited

Loading

mxdvl Aug 23, 2022 •

edited

Loading

mxdvl Aug 23, 2022 •

edited

Loading

JamieB-gu commented Aug 24, 2022 •

edited

Loading