chore: bump node, rethinkdb-ts, typescript, uWS #7780

mattkrick · 2023-02-16T02:13:11Z

Description

Need to bump node for security (plus i want to use lastIndexOf
typescript needs to get bumped to also use lastIndexOf
uWS needs to get bumped to work with node v18
rethinkdb-ts-migrate will need to get bumped because node v18 streams has a getter called closed that conflicts. EDIT: i can just resolve rethinkdb-ts to a higher version & it works great.

Demo

Everything just works 🤞

Test

biggest changes are with uWS, so test refreshing the page, subscriptions, etc.

Signed-off-by: Matt Krick <matt.krick@gmail.com>

…rkflow file

mattkrick · 2023-02-16T20:56:42Z

packages/server/graphql/nestedSchema/nestGitLabEndpoint.ts

@@ -36,7 +36,7 @@ const defaultExecutor: Executor<{
    })
    clearTimeout(timeout)
    const resJSON = (await result.json()) as EndpointExecutionResult | {message?: string}
-    if ('data' in resJSON || 'errors' in resJSON) return resJSON


I think I found a bug in typescript!
...or I'm just an idiot.
Either way, I made an issue for it here: microsoft/TypeScript#52812

oh wow

Let's change the code to the a little bit easier to read equivalent of if ('data' in resJSON || 'errors' in resJSON) return resJSON

if ('data' in resJSON) return resJSON if ('errors' in resJSON) return resJSON

TS complains because data is a mandatory property, and technically the second case indicates the data is not defined.

Let's follow the code execution of if ('data' in resJSON || 'errors' in resJSON) return resJSON

if data is in the resJSON it'll go straight to return resJSON as there's no need to check second condition in the or logical statement if first part is true, the returned type will be { data, errors? }

the second part of the logical or will be executed only if data is not in the resJSON, meaning it'll return a type { errors }, and TS will complain as data is mandatory

Here's what happens in the reversed version ie. if ('errors' in resJSON || 'data' in resJSON) return resJSON

if errors is in the resJSON and type narrowing assumes if the optional property of type is there, then the mandatory has to be there

the second part will be executed only if there are no errors, and if true the returned type will be { data } which is a valid return type

I still get Property 'data' is missing in type '{ message?: string | undefined; } & Record<"errors", unknown>' but required in type 'EndpointExecutionResult'.ts(2322) error on master here but VS code sometimes is misleading so I created a TS playground

Using TS 4.6.4

Using TS 4.9.5

and seems like the way type narrowing works has changed, actually!

mattkrick · 2023-02-16T20:57:50Z

packages/server/socketHandlers/handleClose.ts

-  if (!ws.connectionContext) return
-  ws.done = true
+const handleClose = (ws: WebSocket<SocketUserData>) => {
+  const userData = ws.getUserData()


with the new version of uWS, we can't just assign objects to the WebSocket object. we have to get a UserData object off of it & assign our stuff there.

mattkrick · 2023-02-16T20:58:32Z

packages/server/socketHandlers/handleOpen.ts

 import ConnectionContext from '../socketHelpers/ConnectionContext'
 import keepAlive from '../socketHelpers/keepAlive'
 import {sendEncodedMessage} from '../socketHelpers/sendEncodedMessage'
 import handleConnect from './handleConnect'

 const APP_VERSION = process.env.npm_package_version
+export type SocketUserData = {


tbh, i kinda like that they make us do this. it's more explicit.

mattkrick · 2023-02-16T20:59:12Z

packages/server/socketHandlers/handleUpgrade.ts

@@ -6,7 +6,7 @@ import getQueryToken from '../utils/getQueryToken'
 import sendToSentry from '../utils/sendToSentry'
 import uwsGetIP from '../utils/uwsGetIP'

-const handleUpgrade: WebSocketBehavior['upgrade'] = async (res, req, context) => {
+const handleUpgrade: WebSocketBehavior<void>['upgrade'] = async (res, req, context) => {


during upgrade we don't have any UserData yet, so this is the exception where we use void

BartoszJarocki

I tested everything I can think of locally and seems everything seems to be good!

BartoszJarocki · 2023-02-21T06:45:29Z

CHANGELOG.md

+-

 ### Added

 - chore: Migrate MeetingTemplate to PG (Phase 1 of 3) (#7679)
 - chore(checkout-flow): add checkout feature flag (#7709)
+-

 ### Changed

 - chore: update team invite email (#7710)
 - chore(ai-summary): update meeting summary url (#7705)
+-


+1 no need for empty -

i have no idea why my markdown plugin automatically added those... will fix

BartoszJarocki · 2023-02-21T06:53:14Z

packages/server/dataloader/LocalCache.ts

@@ -171,7 +171,7 @@ export default class LocalCache<T extends keyof CacheType> {
    Object.keys(this.cacheMap).forEach((key) => {
      if (!key.startsWith(`${table}:`)) return
      const doc = this.cacheMap[key]
-      Object.assign(doc, updater)
+      Object.assign(doc as any, updater)


+1 I see TS is complaining about doc being possibly undefined so seems like the correct solution here would be to add {} in this case so Object.assign won't try to write to undefined, am I correct?

const doc = this.cacheMap[key] || {} Object.assign(doc, updater)

not related to PR but this file comment on the top says it's only used for User table but seems like it's only being used for meeting templates, is that something we should fix? also, once we migrate to postgres, is there any need for this functionality?

heh, yeah this file is very old & only used by 1 thing, it will absolutely go away when we move to pg so i didn't take too much time making it perfect. just want it to stop complaining until i can delete it 🙂

BartoszJarocki · 2023-02-21T07:23:03Z

packages/server/graphql/nestedSchema/nestGitLabEndpoint.ts

@@ -36,7 +36,7 @@ const defaultExecutor: Executor<{
    })
    clearTimeout(timeout)
    const resJSON = (await result.json()) as EndpointExecutionResult | {message?: string}
-    if ('data' in resJSON || 'errors' in resJSON) return resJSON


oh wow

Let's change the code to the a little bit easier to read equivalent of if ('data' in resJSON || 'errors' in resJSON) return resJSON

if ('data' in resJSON) return resJSON if ('errors' in resJSON) return resJSON

TS complains because data is a mandatory property, and technically the second case indicates the data is not defined.

Let's follow the code execution of if ('data' in resJSON || 'errors' in resJSON) return resJSON

if data is in the resJSON it'll go straight to return resJSON as there's no need to check second condition in the or logical statement if first part is true, the returned type will be { data, errors? }

the second part of the logical or will be executed only if data is not in the resJSON, meaning it'll return a type { errors }, and TS will complain as data is mandatory

Here's what happens in the reversed version ie. if ('errors' in resJSON || 'data' in resJSON) return resJSON

if errors is in the resJSON and type narrowing assumes if the optional property of type is there, then the mandatory has to be there

the second part will be executed only if there are no errors, and if true the returned type will be { data } which is a valid return type

I still get Property 'data' is missing in type '{ message?: string | undefined; } & Record<"errors", unknown>' but required in type 'EndpointExecutionResult'.ts(2322) error on master here but VS code sometimes is misleading so I created a TS playground

Using TS 4.6.4

Using TS 4.9.5

and seems like the way type narrowing works has changed, actually!

Signed-off-by: Matt Krick <matt.krick@gmail.com>

chore: bump node, rethinkdb-ts, typescript

0efb5bb

mattkrick changed the title ~~chore: bump node, rethinkdb-ts, typescript~~ chore: bump node, rethinkdb-ts, typescript, uWS Feb 16, 2023

github-actions bot added the size/s label Feb 16, 2023

mattkrick added 3 commits February 16, 2023 10:31

chore: fix typecript errors

7a999be

chore: use v18 in gh flow

d67710b

chore: merge master, fix ts errors

4803c33

Signed-off-by: Matt Krick <matt.krick@gmail.com>

github-actions bot added size/m and removed size/s labels Feb 16, 2023

mattkrick and others added 2 commits February 16, 2023 12:25

chore: fix nestGitLabEndpoit types

11918c5

Signed-off-by: Matt Krick <matt.krick@gmail.com>

chore: add node substep to set version in build phase or check.yml wo…

1ffeccd

…rkflow file

adaniels-parabol force-pushed the chore/bump-ts-node branch from e353e68 to 1ffeccd Compare February 16, 2023 22:14

mattkrick commented Feb 16, 2023

View reviewed changes

mattkrick marked this pull request as ready for review February 16, 2023 22:36

mattkrick requested a review from BartoszJarocki February 17, 2023 21:27

BartoszJarocki approved these changes Feb 21, 2023

View reviewed changes

jmtaber129 mentioned this pull request Feb 21, 2023

fix(ci): Fix 'Check Code Style' action #7798

Closed

7 tasks

chore: cleanup changelog

cf6e63d

Signed-off-by: Matt Krick <matt.krick@gmail.com>

mattkrick merged commit 47dfb31 into master Feb 21, 2023

mattkrick deleted the chore/bump-ts-node branch February 21, 2023 19:33

Dschoordsch mentioned this pull request Feb 22, 2023

feat: upgrade node to 16.18.1 #7509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: bump node, rethinkdb-ts, typescript, uWS #7780

chore: bump node, rethinkdb-ts, typescript, uWS #7780

mattkrick commented Feb 16, 2023 •

edited

mattkrick Feb 16, 2023

BartoszJarocki Feb 21, 2023 •

edited

mattkrick Feb 16, 2023

mattkrick Feb 16, 2023

mattkrick Feb 16, 2023

BartoszJarocki left a comment

BartoszJarocki Feb 21, 2023

mattkrick Feb 21, 2023

BartoszJarocki Feb 21, 2023

mattkrick Feb 21, 2023

BartoszJarocki Feb 21, 2023 •

edited

chore: bump node, rethinkdb-ts, typescript, uWS #7780

chore: bump node, rethinkdb-ts, typescript, uWS #7780

Conversation

mattkrick commented Feb 16, 2023 • edited

Description

Demo

Test

Choose a reason for hiding this comment

BartoszJarocki Feb 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BartoszJarocki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BartoszJarocki Feb 21, 2023 • edited

Choose a reason for hiding this comment

mattkrick commented Feb 16, 2023 •

edited

BartoszJarocki Feb 21, 2023 •

edited

BartoszJarocki Feb 21, 2023 •

edited