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

Use esbuild to build server-side code #7809

Merged
merged 7 commits into from
Sep 9, 2022
Merged

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented Aug 18, 2022

Requires #7805

While reviewing #7744 - the pirates changes demonstrated that we should simplify the build process for end developer configurations.

Keystone passes compiling to next/babel at the moment, but that is executed in a way that isn't actually helping us drop the responsibility, nor the maintenance burden. We are operating in a constrained environment, with no configuration options for our users. This could be simpler.

Some options

  • tsc, slower 🐌 , no bundling 😶‍🌫️
  • esbuild, fast 💨, easy bundling 📦, limited configuration ❤

This pull request switches @keystone-6/core to use esbuild for the compilation process.

This change should ideally not break most users, the reality of modules in the JS ecosystem is that every tools does things ever so subtly different so there will likely be small breaks, the most notable change though is that __dirname, __filename, and require.resolve will not be treated specially, this essentially means in most cases, they should just be avoided. If you need to read a file/etc., you should do it relative to process.cwd().

@changeset-bot

This comment was marked as resolved.

@vercel
Copy link

vercel bot commented Aug 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
keystone-next-docs ✅ Ready (Inspect) Visit Preview Sep 9, 2022 at 11:39AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 18, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8cdf849:

Sandbox Source
@keystone-6/sandbox Configuration

@vercel vercel bot temporarily deployed to Preview August 18, 2022 04:52 Inactive
@vercel vercel bot temporarily deployed to Preview August 22, 2022 01:26 Inactive
@dcousens dcousens force-pushed the change-view-resolution branch 2 times, most recently from 5f07b9b to 368583b Compare August 25, 2022 01:47
@vercel vercel bot temporarily deployed to Preview August 25, 2022 02:40 Inactive
Base automatically changed from change-view-resolution to main August 25, 2022 02:50
@jlarmstrongiv
Copy link

jlarmstrongiv commented Aug 25, 2022

Following up from Slack, are the new functions loadBuiltConfig, getBuiltConfigPath, and getAdminPath exported from any part of the @keysone-6/core package (such as @keystone-6/core/system or @keystone-6/core/artifacts)? It’s very important to have those functions exposed.

@dcousens
Copy link
Member

Hi @jlarmstrongiv, I don't think they are and we typically want to be conservative in what we export. Did you have a particular use case in mind?

@jlarmstrongiv
Copy link

@dcousens Ahh, I had missed that initConfig was already exported. I can redefine what’s not included 👍

I’m building my own “start” function to return the underlying express server for experimental aws lambda support and bundling. If I can get that to work, then I should be able to make a pr for an experimental generateExpressApp and a bundling example. Supporting serverless (even just experimentally or a community example) would be very nice and I think it’s doable.

@dcousens dcousens self-assigned this Aug 29, 2022
@vercel vercel bot temporarily deployed to Preview August 30, 2022 05:36 Inactive
@dcousens dcousens marked this pull request as ready for review August 30, 2022 05:57
@vercel vercel bot temporarily deployed to Preview August 30, 2022 07:17 Inactive
@vercel vercel bot temporarily deployed to Preview August 30, 2022 23:13 Inactive
@emmatown
Copy link
Member Author

Before merging this, we should make sure source maps work (we might want to skip source maps for the prod build but they should definitely work in dev)

@vercel vercel bot temporarily deployed to Preview August 31, 2022 04:48 Inactive
@vercel vercel bot temporarily deployed to Preview September 6, 2022 23:29 Inactive
@emmatown emmatown force-pushed the use-esbuild-on-the-server branch 4 times, most recently from 734bad1 to bfbbe40 Compare September 8, 2022 04:15
@vercel vercel bot temporarily deployed to Preview September 8, 2022 04:22 Inactive
@emmatown emmatown marked this pull request as draft September 8, 2022 04:29
@vercel vercel bot temporarily deployed to Preview September 8, 2022 23:25 Inactive
@emmatown
Copy link
Member Author

emmatown commented Sep 8, 2022

Source maps turned into a rabbit hole, doing it Correctly would be very subtle so let's go with no source maps for now rather than wrong source maps. The bundle that esbuild generates is reasonably readable so I don't think it's a huge concern.

@vercel vercel bot temporarily deployed to Preview September 9, 2022 03:24 Inactive
@vercel vercel bot temporarily deployed to Preview September 9, 2022 03:30 Inactive
@vercel vercel bot temporarily deployed to Preview September 9, 2022 03:40 Inactive
@vercel vercel bot temporarily deployed to Preview September 9, 2022 03:45 Inactive
@vercel vercel bot temporarily deployed to Preview September 9, 2022 05:44 Inactive
@emmatown emmatown marked this pull request as ready for review September 9, 2022 05:51
@emmatown emmatown requested a review from a team September 9, 2022 05:51
@vercel vercel bot temporarily deployed to Preview September 9, 2022 11:39 Inactive
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.

3 participants