-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Changes from 9 commits
962aae0
8029635
5a3fc28
3d5e4b9
6ec83ed
1d6ad57
1ed82db
d98a81e
7691171
c5ef5fb
4e484ce
7098670
c7fb8b8
5d40891
2efe78a
fdbcda7
5bfb400
2b2404c
9c98ce4
fbf166e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,8 +20,8 @@ type PrimitiveJSONValue = string | number | boolean | undefined | null | |
|
|
||
| export interface JSONArray extends Array<JSONValue> {} | ||
|
|
||
| type SerializableJSONValue = | ||
| | Symbol | ||
| export type SerializableJSONValue = | ||
| | symbol | ||
| | Set<SuperJSONValue> | ||
| | Map<SuperJSONValue, SuperJSONValue> | ||
| | undefined | ||
|
|
@@ -30,14 +30,14 @@ type SerializableJSONValue = | |
| | RegExp | ||
|
|
||
| // Here's where we excluded `ClassInstance` (which was `any`) from the union. | ||
| type SuperJSONValue = | ||
| export type SuperJSONValue = | ||
| | JSONValue | ||
| | SerializableJSONValue | ||
| | SuperJSONArray | ||
| | SuperJSONObject | ||
|
|
||
| interface SuperJSONArray extends Array<SuperJSONValue> {} | ||
| export interface SuperJSONArray extends Array<SuperJSONValue> {} | ||
|
|
||
| interface SuperJSONObject { | ||
| export interface SuperJSONObject { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep, exactly.
Yep me too. I probably moved some stuff around and this is no longer relevant. I can undo if you want.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops just remembered, it is only visible in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should really remove the |
||
| [key: string]: SuperJSONValue | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,14 @@ | ||
| import { OAuth2Provider, OAuth2ProviderWithPKCE } from "arctic"; | ||
|
|
||
| export function defineProvider< | ||
| OAuthClient extends OAuth2Provider | OAuth2ProviderWithPKCE | ||
| OAuthClient extends OAuth2Provider | OAuth2ProviderWithPKCE, | ||
| const Id extends string | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice improvement! |
||
| >({ | ||
| id, | ||
| displayName, | ||
| oAuthClient, | ||
| }: { | ||
| id: string; | ||
| id: Id; | ||
| displayName: string; | ||
| oAuthClient: OAuthClient; | ||
| }) { | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did test
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so. But @infomiho can confirm
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ import type { | |
| _{= crud.entityUpper =}, | ||
| } from "../_types"; | ||
| import type { Prisma } from "@prisma/client"; | ||
| import type { Payload } from "../_types/serialization"; | ||
| import type { Payload, SuperJSONObject } from "../_types/serialization"; | ||
| import type { | ||
| {= crud.entityUpper =}, | ||
| } from "wasp/entities"; | ||
|
|
@@ -37,7 +37,7 @@ type _WaspEntity = {= crud.entityUpper =} | |
| /** | ||
| * PUBLIC API | ||
| */ | ||
| export namespace {= crud.name =} { | ||
| export declare namespace {= crud.name =} { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, can't get this error either. What threw it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing, just the recommendations on TypeScript's handbook. If we just do |
||
| {=# crud.operations.GetAll =} | ||
| export type GetAllQuery<Input extends Payload = never, Output extends Payload = Payload> = {= queryType =}<[_WaspEntityTagged], Input, Output> | ||
| {=/ crud.operations.GetAll =} | ||
|
|
@@ -61,7 +61,7 @@ export namespace {= crud.name =} { | |
|
|
||
| /** | ||
| * PRIVATE API | ||
| * | ||
| * | ||
| * The types with the `Resolved` suffix are the types that are used internally by the Wasp client | ||
| * to implement full-stack type safety. | ||
| */ | ||
|
|
@@ -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 | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| type GetOutput = _WaspEntity | null | ||
| export type GetQueryResolved = {= crud.name =}.GetQuery<GetInput, GetOutput> | ||
| {=/ overrides.Get.isDefined =} | ||
|
|
@@ -91,7 +91,7 @@ export type GetQueryResolved = typeof _waspGetQuery | |
|
|
||
| {=# crud.operations.Create =} | ||
| {=^ overrides.Create.isDefined =} | ||
| type CreateInput = Prisma.XOR< | ||
| type CreateInput = SuperJSONObject & Prisma.XOR< | ||
| Prisma.{= crud.entityUpper =}CreateInput, | ||
| Prisma.{= crud.entityUpper =}UncheckedCreateInput | ||
| > | ||
|
|
@@ -106,7 +106,7 @@ export type CreateActionResolved = typeof _waspCreateAction | |
|
|
||
| {=# crud.operations.Update =} | ||
| {=^ overrides.Update.isDefined =} | ||
| type UpdateInput = Prisma.XOR< | ||
| type UpdateInput = SuperJSONObject & Prisma.XOR< | ||
| Prisma.{= crud.entityUpper =}UpdateInput, | ||
| Prisma.{= crud.entityUpper =}UncheckedUpdateInput | ||
| > | ||
|
|
@@ -123,7 +123,7 @@ export type UpdateActionResolved = typeof _waspUpdateAction | |
|
|
||
| {=# crud.operations.Delete =} | ||
| {=^ overrides.Delete.isDefined =} | ||
| type DeleteInput = Prisma.{= crud.entityUpper =}WhereUniqueInput | ||
| type DeleteInput = SuperJSONObject & Prisma.{= crud.entityUpper =}WhereUniqueInput | ||
| type DeleteOutput = _WaspEntity | ||
| export type DeleteActionResolved = {= crud.name =}.DeleteAction<DeleteInput, DeleteOutput> | ||
| {=/ overrides.Delete.isDefined =} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ import { | |
| serialize as superjsonSerialize, | ||
| } from 'superjson' | ||
| import { handleRejection } from 'wasp/server/utils' | ||
| {=# isAuthEnabled =} | ||
| import { makeAuthUserIfPossible } from 'wasp/auth/user' | ||
| {=/ isAuthEnabled =} | ||
|
Comment on lines
+4
to
+6
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| export function createOperation (handlerFn) { | ||
| return handleRejection(async (req, res) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ | |
|
|
||
| import http from 'http' | ||
| import { Server, Socket } from 'socket.io' | ||
| import type { ServerType } from 'wasp/server/webSocket' | ||
|
|
||
| import { config, prisma } from 'wasp/server' | ||
|
|
||
|
|
@@ -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, { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We we're not using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The import { webSocketFn as webSocketFn_ext } from 'wasp/src/webSocket'
// ...
export type ServerType = Parameters<WebSocketFn>[0]
// ...
type WebSocketFn = typeof webSocketFn_extwhere the 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 It would be great if we can figure it out to have some extra type safety e.g. when users will import the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found the error,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| cors: { | ||
| origin: config.frontendUrl, | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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
Stringinstead ofstringThere 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.
Uh oh!
There was an error while loading. Please reload this page.
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: ravionhq/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
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 😂