-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
In CI, check for TS errors in built server code (of example app) by running npx tsc
#2672
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
Conversation
e23663d
to
488013d
Compare
…unning `npx tsc` Fixes #2384
488013d
to
8029635
Compare
This comment was marked as outdated.
This comment was marked as outdated.
type SerializableJSONValue = | ||
| Symbol | ||
export type SerializableJSONValue = | ||
| symbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixing a TS linting warning here, it's because of the same reason why we shouldn't use String
instead of string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems right to me...
But I copied this over from superjson (link in the comment above). And they still have Symbol
. Can you open an issue there and see whether we're missing something?
Or you'll help them find a bug - win win 😄
Oh, and what threw this error? I'm not getting anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a PR, let's see if they have any complaints: flightcontrolhq/superjson#318
The TypeScript Handbook explicitly calls this out so I'm not sure there's a reason for not merging it: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#number-string-boolean-symbol-and-object
Oh, and what threw this error? I'm not getting anything.
When I wrote this it was because I thought I got the linting warning. Apparently the linter was my own brain 'cause I can't get it again 😂
export type SuperJSONValue = | ||
| JSONValue | ||
| SerializableJSONValue | ||
| SuperJSONArray | ||
| SuperJSONObject | ||
|
||
interface SuperJSONArray extends Array<SuperJSONValue> {} | ||
export interface SuperJSONArray extends Array<SuperJSONValue> {} | ||
|
||
interface SuperJSONObject { | ||
export interface SuperJSONObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to export
these types because other types make reference to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, which ones?
I can't find any explicit references, so I'm guessing you meant that some types implicitly depend on them when creating declarations (the "type cannot be named without a reference to..." error).
So I've tried removing the exports from SuperJSONArray
, SuperJSONValue
, and SerializableJsonValue
, rebuilding the SDK, and rebuilding the server. Everything seems to work OK.
Not that exporting these types is a problem (perhaps it's a good thing to do even if it doesn't solve anything), but I'd like to understand what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find any explicit references, so I'm guessing you meant that some types implicitly depend on them when creating declarations (the "type cannot be named without a reference to..." error).
Yep, exactly.
So I've tried removing the exports from SuperJSONArray, SuperJSONValue, and SerializableJsonValue, rebuilding the SDK, and rebuilding the server. Everything seems to work OK.
Yep me too. I probably moved some stuff around and this is no longer relevant. I can undo if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops just remembered, it is only visible in crud-testing
:
🐝 --- Building SDK... ------------------------------------------------------------
[ Wasp ] client/crud/tasks.ts(58,14): error TS4023: Exported variable 'tasks' has or is using name 'SuperJSONArray' from external module "/Users/carlos/Developer/Wasp/wasp/waspc/examples/crud-testing/.wasp/out/sdk/wasp/server/_types/serialization" but cannot be named.
[ Wasp ] client/crud/tasks.ts(58,14): error TS4023: Exported variable 'tasks' has or is using name 'SuperJSONObject' from external module "/Users/carlos/Developer/Wasp/wasp/waspc/examples/crud-testing/.wasp/out/sdk/wasp/server/_types/serialization" but cannot be named.
[ Wasp ] client/crud/tasks.ts(58,14): error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should really remove the crud-testing
app and move the code to todoApp
so we don't have to test out with two different apps. We'll do it when we figure out all example apps.
@@ -1,13 +1,14 @@ | |||
import { OAuth2Provider, OAuth2ProviderWithPKCE } from "arctic"; | |||
|
|||
export function defineProvider< | |||
OAuthClient extends OAuth2Provider | OAuth2ProviderWithPKCE | |||
OAuthClient extends OAuth2Provider | OAuth2ProviderWithPKCE, | |||
const Id extends string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the const
type parameter makes it output a e.g. {id: "discord"}
type instead of a {id: string}
one. So then it typechecks correctly for the login provider actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
@@ -37,7 +37,7 @@ type _WaspEntity = {= crud.entityUpper =} | |||
/** | |||
* PUBLIC API | |||
*/ | |||
export namespace {= crud.name =} { | |||
export declare namespace {= crud.name =} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing a TS lint, namespaces and modules should not be mixed in the runtime time, so we make it only a type thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, can't get this error either. What threw it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing, just the recommendations on TypeScript's handbook. If we just do export declare
there's no output, so no interplay between JS modules and namespaces.
@@ -79,7 +79,7 @@ export type GetAllQueryResolved = typeof _waspGetAllQuery | |||
|
|||
{=# crud.operations.Get =} | |||
{=^ overrides.Get.isDefined =} | |||
type GetInput = Prisma.{= crud.entityUpper =}WhereUniqueInput | |||
type GetInput = SuperJSONObject & Prisma.{= crud.entityUpper =}WhereUniqueInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Prisma accepts inputs that are not serializable (e.g. functions), so this intersections makes it select only the kinds of inputs that are.
Otherwise it will fail in the following line as Wasp requires only SuperJSON-serializable inputs.
{=# isAuthEnabled =} | ||
import { makeAuthUserIfPossible } from 'wasp/auth/user' | ||
{=/ isAuthEnabled =} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasp/auth/user
doesn't exist if auth is not enabled
@@ -17,7 +16,7 @@ import { makeAuthUserIfPossible } from 'wasp/auth/user' | |||
export async function init(server: http.Server): Promise<void> { | |||
// TODO: In the future, we can consider allowing a clustering option. | |||
// Ref: https://github.com/wasp-lang/wasp/issues/1228 | |||
const io: ServerType = new Server(server, { | |||
const io = new Server(server, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We we're not using ServerType
anywhere (it's not returned from the function or anything), this line and the following lines were failing if it wasn't a socket.io Server
, so I just removed the type annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infomiho Please comment on this one.
I'm guessing we wanted a relationship between SocketIO and what the user's function receives. But something seems to be missing. This type should have been connected to WebSocketDefinition
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ServerType
is defined based on the user defined handler:
import { webSocketFn as webSocketFn_ext } from 'wasp/src/webSocket'
// ...
export type ServerType = Parameters<WebSocketFn>[0]
// ...
type WebSocketFn = typeof webSocketFn_ext
where the wasp/src/webSocket
exports:
export const webSocketFn: WebSocketDefinition<
ClientToServerEvents,
ServerToClientEvents,
InterServerEvents
> = (io, context) => {
// ...
}
interface ServerToClientEvents {
chatMessage: (msg: { id: string; username: string; text: string }) => void
}
interface ClientToServerEvents {
chatMessage: (msg: string) => void
}
interface InterServerEvents {}
It basically takes the io
param and gets its type based on the user defined events. Why this causes a type error - I'm not sure. Maybe there is something off with the way we infer the ServerType
type.
It would be great if we can figure it out to have some extra type safety e.g. when users will import the io
instance in their server code (currently not possible directly, see: #1289). If it's not possible / too much to do now, let's comment on the issue I linked and solve it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the error, engine.io
version mismatch in todo-typescript
between user code and .wasp/build/server
, I'll see how I can fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @infomiho let's try to sync up on this because I have some questions on how we could solve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking with Miho, I created this issue #2726. While we fix it, we agreed that the best course of action would be to work around it by removing the type annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
This is a very welcome change. It has potential to be much more useful than just running npx tsc
in waspc/todoApp
.
We can easily extend it to resolve #2458. Seems like all that's left is:
- Reactivating typescript compilation in server's
package.json
- Double-checking CRUD (the only feature not covered by
waspc/todoApp
).
If we don't do that now and keep TypeScript "off" for the server code, we'll likely introduce new errors.
So... Can we do it? :)
type SerializableJSONValue = | ||
| Symbol | ||
export type SerializableJSONValue = | ||
| symbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems right to me...
But I copied this over from superjson (link in the comment above). And they still have Symbol
. Can you open an issue there and see whether we're missing something?
Or you'll help them find a bug - win win 😄
Oh, and what threw this error? I'm not getting anything.
export type SuperJSONValue = | ||
| JSONValue | ||
| SerializableJSONValue | ||
| SuperJSONArray | ||
| SuperJSONObject | ||
|
||
interface SuperJSONArray extends Array<SuperJSONValue> {} | ||
export interface SuperJSONArray extends Array<SuperJSONValue> {} | ||
|
||
interface SuperJSONObject { | ||
export interface SuperJSONObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, which ones?
I can't find any explicit references, so I'm guessing you meant that some types implicitly depend on them when creating declarations (the "type cannot be named without a reference to..." error).
So I've tried removing the exports from SuperJSONArray
, SuperJSONValue
, and SerializableJsonValue
, rebuilding the SDK, and rebuilding the server. Everything seems to work OK.
Not that exporting these types is a problem (perhaps it's a good thing to do even if it doesn't solve anything), but I'd like to understand what's going on.
@@ -1,13 +1,14 @@ | |||
import { OAuth2Provider, OAuth2ProviderWithPKCE } from "arctic"; | |||
|
|||
export function defineProvider< | |||
OAuthClient extends OAuth2Provider | OAuth2ProviderWithPKCE | |||
OAuthClient extends OAuth2Provider | OAuth2ProviderWithPKCE, | |||
const Id extends string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes make sense but please test them if you haven't (the waspc/examples/todoApp
doesn't have CRUD so it might have slipped through the cracks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did test crud-testing
, do you think that covered it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. crud-testing
would be a pretty bad name if it didn't :)
But @infomiho can confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, crud-testing
covers the CRUD feature 👍
@@ -17,7 +16,7 @@ import { makeAuthUserIfPossible } from 'wasp/auth/user' | |||
export async function init(server: http.Server): Promise<void> { | |||
// TODO: In the future, we can consider allowing a clustering option. | |||
// Ref: https://github.com/wasp-lang/wasp/issues/1228 | |||
const io: ServerType = new Server(server, { | |||
const io = new Server(server, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@infomiho Please comment on this one.
I'm guessing we wanted a relationship between SocketIO and what the user's function receives. But something seems to be missing. This type should have been connected to WebSocketDefinition
.
I'll take a look at this PR tomorrow 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition, left a comment related to the io
type, let's figure that before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing and I think we're good to go!
@sodic ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more questions, but I think this is the final iteration :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, awesome work!
Glad this is finally working. It was a big personal frustration for me :)
@@ -10,5 +10,6 @@ export const SERIALIZABLE_OBJECTS_FIXTURE = { | |||
negInf: -Infinity, | |||
// we ensure that it'd be too big to be represented as a `number` | |||
decimal: new Prisma.Decimal(Number.MAX_SAFE_INTEGER).mul(2), | |||
// @ts-ignore Server has a tsconfig target of es2017, so BigInt isn't available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the server tsconfig? We can do it in a different PR and remove this comment.
If it turns out to cause problems, we can create an issue and link it here.
Resolves #2384
Now in CI we type-check the built server and client of
waspc/examples/todoApp
. I had to modify some types to make it all pass.I also compiled and tested the other
waspc/example/*
apps and prompted me to fix some more typesSelect what type of change this PR introduces: