Skip to content
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

Bundling server with Rollup + esbuild #1714

Merged
merged 9 commits into from
Feb 7, 2024
Merged

Bundling server with Rollup + esbuild #1714

merged 9 commits into from
Feb 7, 2024

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Feb 1, 2024

We wanted to tackle the issue of requiring our users to always use .js extension in the Wasp external imports.

One of the directions we can take is to bundle the server with e.g. Rollup and delegate the module resolution to the bundler.

Note: I've removed tsc in the server since we are using esbuild to compile TS. We can return tsc --noEmit if needed to also perform type checks.

Signed-off-by: Mihovil Ilakovac <[email protected]>
@@ -40,33 +40,3 @@ async (req: RequestWithExtraFields, res: Response, next: NextFunction) => {
}

export const sleep = (ms: number): Promise<unknown> => new Promise((r) => setTimeout(r, ms))

export function getDirPathFromFileUrl(fileUrl: string): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this code relied on importing from a specific directory e.g. config and bundling gets rid of the folder structure and bundles everything in one file that we can't use this method anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, so we lose dynamic imports, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we used the dynamic imports based on folder structure, but now there is not folder structure we can rely on. All of the code is bundled together. Not a big deal, we used it only for auth provider imports.

@@ -0,0 +1,37 @@
import esbuild from 'rollup-plugin-esbuild'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uses Rollup and the esbuild plugin (which is fast and that's want we want in dev)

];
const providers = await importProviders(whitelistedProviderConfigFileNames);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -41,7 +41,7 @@ app TodoTypescript {
system: PostgreSQL
},
webSocket: {
fn: import { webSocketFn } from "@src/websocket/index.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing extensions and it keeps working 🎉

Copy link
Contributor Author

@infomiho infomiho Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an issue with a different behaviour in our LSP and what our code will tolerate.

Screenshot 2024-02-05 at 17 06 53
LSP doesn't recognise imports from folders, BUT this will run fine though
Screenshot 2024-02-05 at 17 08 04
Adding index helps (this will also run fine)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is tricky. DO you mind opening an issue for it? You can mark it with restructuring. I think we can handle this one after 0.12 is out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created an issue for us to tackle this later #1717


enabledProviderIds =
providers =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick and dirty change to get the whole server working. Maybe this code will need refactoring later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas how you would refactor it, or what is not great now? Might help docuemting this now while it is fresh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take another look now at the code, so I'm okay with what we merge in this PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored it a bit in d2d1b4d

Now it feels fine 👍

"build-and-start": "npm run build && npm run start",
"watch": "nodemon --exec 'npm run build-and-start || exit 1'",
"bundle": "rollup --config --silent",
"start": "npm run validate-env && NODE_PATH=dist node --enable-source-maps -r dotenv/config bundle/server.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using source maps for better error reporting 👍

sourcemap: true,
},
],
external: (id) => !/^[./]/.test(id),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this pattern in the wild, it basically means: anything that's not a relative import, it's a dependency and don't try to bundle it together. https://rollupjs.org/configuration-options/#external

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! We should certainly provide this comment inside this file, since it won't be obvious.

@Martinsos
Copy link
Member

Martinsos commented Feb 2, 2024

We wanted to tackle the issue of requiring our users to always use .js extension in the Wasp external imports.

One of the directions we can take is to bundle the server with e.g. Rollup and delegate the module resolution to the bundler.

Note: I've removed tsc in the server since we are using esbuild to compile TS. We can return tsc --noEmit if needed to also perform type checks.

So it doesn't do typechecking in the terminal any more? That sucks a bit. I know Vite does same thing, but I also think that sucks. I know IDE does the typechecking, but I love (and some other people in Discord said the same, or at least one person) when the actual tool that is running my development also let's me know about errors, so it works even if I don't open it in editor and search for type errors. I want to know in terminal all is fine, from the "credible" source. But you said we can return tsc --noEmit then?

@infomiho infomiho changed the title Prototype of server bundling Bundling server with Rollup + esbuild Feb 5, 2024
Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infomiho left a bit of comments, but I will let @sodic do the real review and approve this, since he has more knowledge than me. But I like the change!


enabledProviderIds =
providers =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas how you would refactor it, or what is not great now? Might help docuemting this now while it is fresh.

@@ -40,33 +40,3 @@ async (req: RequestWithExtraFields, res: Response, next: NextFunction) => {
}

export const sleep = (ms: number): Promise<unknown> => new Promise((r) => setTimeout(r, ms))

export function getDirPathFromFileUrl(fileUrl: string): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, so we lose dynamic imports, right?

@@ -41,7 +41,7 @@ app TodoTypescript {
system: PostgreSQL
},
webSocket: {
fn: import { webSocketFn } from "@src/websocket/index.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is tricky. DO you mind opening an issue for it? You can mark it with restructuring. I think we can handle this one after 0.12 is out.

@infomiho
Copy link
Contributor Author

infomiho commented Feb 6, 2024

Here's an issue to introduce type checking to the dev setup: #1718

@infomiho infomiho merged commit b23e4ce into main Feb 7, 2024
1 of 4 checks passed
@infomiho infomiho deleted the miho-bundle-server branch February 7, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants