Add `app` and `server` to server `setupFn` #1017

shayneczyzewski · 2023-02-20T20:50:04Z

Description

This PR adds app and server to the server setupFn, mainly as an escape hatch. Users can then do arbitrary Express endpoints, or things like websockets.

It is the start of #268, but the real finish there is adding some proper api DSL support.

Select what type of change this PR introduces:

Just code/docs improvement (no functional change).
Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Update Waspc ChangeLog and version if needed

If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following:

I updated ChangeLog.md with description of the change this PR introduces.
I bumped waspc version in waspc.cabal to reflect changes I introduced, with regards to the version of the latest wasp release, if the bump was needed.

infomiho · 2023-02-20T22:08:16Z

waspc/examples/todoApp/src/server/serverSetup.js

@@ -5,7 +5,8 @@ let someResource = undefined

 export const getSomeResource = () => someResource

-const setup = async () => {
+const setup = async (app, _server) => {


I would recommend adding a type for the ServerSetupFn like:

type ServerSetupFn = (context: ServerSetupFnContext) => void; type ServerSetupFnContext = { app: express.Application, server: http.Server, };

which the user can import like import { ServerSetupFnContext } from '@wasp/types'; or similar.

Yep, can do. 👍🏻 This is just a WIP skeleton at the moment but will beef it out tomorrow some more before tagging for reviews.

Added some types, thanks!

shayneczyzewski · 2023-02-22T09:04:43Z

waspc/data/Generator/templates/server/src/types/index.ts

+export type Application = express.Application
+
+export type Server = http.Server


Thoughts on exporting type aliases like these? I would like people to be able to pass around/use the components of ServerSetupFnContext and still have proper typing, but without having to import the actual express/node imports themselves. I think it makes it nice they can get it all from the same import path, but curious on the TS expert pros/cons here. The downside is they will still need to know it is actually an express.Application if they want to use it with other libraries, look up docs, etc. but I hope that is an ok use for aliases? Thanks!

For example, see function addCustomRoute(app: Application) in serverSetup.ts.

I like the reexport in our context, so users can import from @wasp/types 👍

I would maybe write it a bit differently:

import { Application } from 'express'; import { Server } from 'http'; export type ServerSetupFn = (context: ServerSetupFnContext) => Promise<void> export type ServerSetupFnContext = { app: Application, server: Server, } export { Application } from 'express'; export { Server } from 'http';

Ok I will leave this to @sodic and @infomiho .
I need to take some time these days to learn TS but it is not yet that day :D!

Sounds good, thanks @infomiho for a better convention to use!

infomiho · 2023-02-22T09:14:30Z

waspc/data/Generator/templates/server/src/server.ts

  {=# doesServerSetupFnExist =}
-  await {= serverSetupJsFnIdentifier =}()
+  const serverSetupFnContext: ServerSetupFnContext = { app, server }


@shayneczyzewski Why are we exposing the server object? I'm not sure for which use case it's needed

Many websocket libraries wrap the server. For example: https://socket.io/get-started/chat#integrating-socketio That was one main use case I was hoping to cover here as well (and that won't be covered by the api DSL next step).

Makes sense, I though that socket.io didn't need the server 🤦‍♂️

When I was doing this for an experiment, I did it as described here: https://discord.com/channels/686873244791210014/920312576133443634/1070052697572638870 -> so I passed app to setupFn, but I didn't move the server creation above it and then pass it.

I think it is cool that you did, that gives people more stuff to work with!

But it does change behavior a bit -> before, setupFn was allowing you to do stuff before server is created. Now, it allows you to set up stuff after server is created. I have no idea if we loose anything with that? Is there a use case where it might be important to do something before server is created from app? If we are not sure now, that is ok, we can just go with this and if we learn there is use case for it, we can add another function called preSetupFn or smth, that will be called just before the server.

One thing I thought was important was to add routes to app before the server(app) is called, but I guess I was wrong because I am sure you tested this and it works, even though you add stuff to app after the server(app) is called. Interesting. But does it work because it should, or by accident?

Maybe we can get some answers here: https://nodejs.org/api/http.html#class-httpserver .

Good question @Martinsos - I don't think there is any technical reason why we have to keep it after, but you are right it does change the previous implied semantics a bit from user-level docs (which I massaged to say "before the server starts accepting requests"). But I don't think there is any actual technical impact in doing this swap. The custom route I setup in the setupFn still worked even when createServer happened before. I am not aware of any other potential impacts of this.

I believe createServer is just registering a request handler callback (of which an Express instance itself is). That callback should be using things internally that can be set either before or after that registration (i.e. it shouldn't even know it was registered to me). But I will do more testing and digging to be 100% sure (including doing a Socket.IO test too).

Sounds good @shayneczyzewski !

It's not pretty (we have to specify the client/server URLs since different domains) but you can get Socket.IO up and running with access to server: https://gist.github.com/shayneczyzewski/165b4266d77a2a75454f605dd55b5db7

Martinsos

@shayneczyzewski this is it from me, I will let @sodic or @infomiho handle the TS side of this!

Martinsos · 2023-02-22T14:00:57Z

waspc/data/Generator/templates/server/src/server.ts

  {=# doesServerSetupFnExist =}
-  await {= serverSetupJsFnIdentifier =}()
+  const serverSetupFnContext: ServerSetupFnContext = { app, server }


When I was doing this for an experiment, I did it as described here: https://discord.com/channels/686873244791210014/920312576133443634/1070052697572638870 -> so I passed app to setupFn, but I didn't move the server creation above it and then pass it.

I think it is cool that you did, that gives people more stuff to work with!

But it does change behavior a bit -> before, setupFn was allowing you to do stuff before server is created. Now, it allows you to set up stuff after server is created. I have no idea if we loose anything with that? Is there a use case where it might be important to do something before server is created from app? If we are not sure now, that is ok, we can just go with this and if we learn there is use case for it, we can add another function called preSetupFn or smth, that will be called just before the server.

One thing I thought was important was to add routes to app before the server(app) is called, but I guess I was wrong because I am sure you tested this and it works, even though you add stuff to app after the server(app) is called. Interesting. But does it work because it should, or by accident?

Maybe we can get some answers here: https://nodejs.org/api/http.html#class-httpserver .

Martinsos · 2023-02-22T14:14:23Z

waspc/data/Generator/templates/server/src/types/index.ts

+export type Application = express.Application
+
+export type Server = http.Server


Ok I will leave this to @sodic and @infomiho .
I need to take some time these days to learn TS but it is not yet that day :D!

Martinsos

LGTM from me, will let @infomiho and/or @sodic comment on TS stuff.

web/docs/language/features.md

Co-authored-by: Mihovil Ilakovac <mihovil@ilakovac.com>

shayneczyzewski added 3 commits February 15, 2023 12:48

start of setupFn enhancements

a3d650b

we will just create server before and pass in

aec4011

Merge branch 'main' into shayne-add-app-to-setupFn

f83c532

infomiho reviewed Feb 20, 2023

View reviewed changes

shayneczyzewski added 3 commits February 21, 2023 14:20

exporting some types

df0b437

add more types for use in setupFn

766cdda

adding some doc examples

a11b9b7

shayneczyzewski commented Feb 22, 2023

View reviewed changes

infomiho reviewed Feb 22, 2023

View reviewed changes

Martinsos reviewed Feb 22, 2023

View reviewed changes

Martinsos approved these changes Feb 23, 2023

View reviewed changes

infomiho reviewed Feb 23, 2023

View reviewed changes

web/docs/language/features.md Outdated Show resolved Hide resolved

shayneczyzewski and others added 2 commits February 23, 2023 09:44

Update web/docs/language/features.md

2a836e6

Co-authored-by: Mihovil Ilakovac <mihovil@ilakovac.com>

PR feedback and header manipulation example

a541623

shayneczyzewski changed the title ~~WIP: Add app and server to server setupFn~~ Add app and server to server setupFn Feb 24, 2023

shayneczyzewski marked this pull request as ready for review February 24, 2023 12:48

infomiho approved these changes Feb 27, 2023

View reviewed changes

shayneczyzewski and others added 4 commits February 27, 2023 11:28

update changelog

b308a44

Merge branch 'main' into shayne-add-app-to-setupFn

054829e

e2e

517fe5d

tweak changelog

cc7b208

shayneczyzewski merged commit 8991043 into main Feb 27, 2023

shayneczyzewski deleted the shayne-add-app-to-setupFn branch February 27, 2023 17:19

infomiho mentioned this pull request Feb 28, 2023

Defining a serverSetup function without args crashes the server #1035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `app` and `server` to server `setupFn` #1017

Add `app` and `server` to server `setupFn` #1017

shayneczyzewski commented Feb 20, 2023 •

edited

infomiho Feb 20, 2023

shayneczyzewski Feb 20, 2023 •

edited

shayneczyzewski Feb 22, 2023

shayneczyzewski Feb 22, 2023

shayneczyzewski Feb 22, 2023

infomiho Feb 22, 2023 •

edited

Martinsos Feb 22, 2023

shayneczyzewski Feb 22, 2023

infomiho Feb 22, 2023

shayneczyzewski Feb 22, 2023 •

edited

infomiho Feb 22, 2023

Martinsos Feb 22, 2023

shayneczyzewski Feb 22, 2023

Martinsos Feb 23, 2023

shayneczyzewski Feb 23, 2023

Martinsos left a comment

Martinsos Feb 22, 2023

Martinsos Feb 22, 2023

Martinsos left a comment

		export type Application = express.Application

		export type Server = http.Server

Add app and server to server setupFn #1017

Add app and server to server setupFn #1017

Conversation

shayneczyzewski commented Feb 20, 2023 • edited

Description

Select what type of change this PR introduces:

Update Waspc ChangeLog and version if needed

Choose a reason for hiding this comment

shayneczyzewski Feb 20, 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

infomiho Feb 22, 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

shayneczyzewski Feb 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

Add `app` and `server` to server `setupFn` #1017

Add `app` and `server` to server `setupFn` #1017

shayneczyzewski commented Feb 20, 2023 •

edited

shayneczyzewski Feb 20, 2023 •

edited

infomiho Feb 22, 2023 •

edited

shayneczyzewski Feb 22, 2023 •

edited