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

Added ESBuild for building app and bundling/minfiying assets #375

Merged
merged 18 commits into from
Jun 17, 2024

Conversation

Thomas-Geraghty
Copy link
Contributor

@Thomas-Geraghty Thomas-Geraghty commented Jun 13, 2024

  • Added ESBuild for building project
  • Added support for minifying and bundling client side assets
  • Replaced nodemon with chokidar for watching/building project assets

Note: The base for this PR is a branch where I've moved the assets folder to /server/assets, probably not necessary in retrospect. It makes this build process a bit tidier :)

psoleckimoj
psoleckimoj previously approved these changes Jun 13, 2024
Copy link
Contributor

@psoleckimoj psoleckimoj left a comment

Choose a reason for hiding this comment

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

LGTM!

esbuild/assets.config.js Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
server/views/partials/layout.njk Show resolved Hide resolved
@Thomas-Geraghty Thomas-Geraghty changed the base branch from move-assets-folder to main June 17, 2024 11:35
@Thomas-Geraghty Thomas-Geraghty dismissed psoleckimoj’s stale review June 17, 2024 11:35

The base branch was changed.

@Thomas-Geraghty Thomas-Geraghty marked this pull request as ready for review June 17, 2024 11:49
Dockerfile Show resolved Hide resolved
andrewrlee
andrewrlee previously approved these changes Jun 17, 2024
Copy link
Contributor

@andrewrlee andrewrlee left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@andrewrlee andrewrlee left a comment

Choose a reason for hiding this comment

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

LGTM

@Thomas-Geraghty Thomas-Geraghty merged commit 9d4c1ac into main Jun 17, 2024
4 checks passed
@ushkarev
Copy link
Contributor

ushkarev commented Jul 10, 2024

@Thomas-Geraghty when cherry-picking this to our projects…

  1. it was actually pretty easy to merge 🎉
  2. i noticed a few changes that might be worthwhile:
    • nodemon and sass direct dev dependencies can be removed from package.json
    • there's no longer a need to persist assets/stylesheets in the CircleCI workflow nor git-ignore that path
    • (this is probs more personal style) swapping console.log(…) for process.stderr.write(…) and then we don't need the "no-console" eslint exemption
    • asset-building script logs caught exceptions, but app-building exited without info
    • i don't think static asset routes are needed to these paths: /dist/assets/stylesheets & /dist/assets/js – i know you just rewrote the existing ones to add the /dist prefix, but it feels tidier to not have multiple routes to the same files
    • i tried (and failed) to turn the build scripts into typescript, but instead added some half-arsed type annotations so at least an IDE helps even if tsc doesn't enforce things

which, if any, of these bullet points do you like and want me to copy back upstream?

@Thomas-Geraghty
Copy link
Contributor Author

Thanks for testing this out @ushkarev and getting it integrated into another project! I know @ReedSoftware has integrated it into one of their projects too so he might have some additional fixes 😄

Just to address your points:

  • 2.1 - Great! There's actually a few dependencies and leftover script stuff that I've removed on the projects I've integrated this into, but not backported here. Sounds like we can combine them both.
  • 2.3 - Yep there's definitely an issue with certain errors causing the build/watch server to fail, and not being logged correctly. Also got fixes for these that I need to backport. I'd be interested to see what changes you'd made, ours look like this
  • 2.4 - Agreed, let's clean those up! 🚀
  • 2.5 - Type annotations are a good idea 😄 I think if we did want the config as Typescript, we'd need ANOTHER compiler to turn it back into JavaScript to run it 🤦 no tidy way around I don't think 😭

One of the major changes we've implemented on some of my projects is building one single file for the whole express app, and then building a sourcemap for it. In the current template implementation, every file outputted for the app ends up with a bunch of ESM/cJS converting logic, building one output file means there's just one instance and our end result is much neater - with sourcemaps continuing to allow for the same level of debugging.

I'd be happy to backport this if people are comfortable, we're using it on 3 different projects without any concerns.

@ReedSoftware
Copy link
Contributor

@Thomas-Geraghty from our side, the only thing we've done is add an extra entry to app.copy array because our project structure is slightly different, but that was an easy 10 second job. I will echo your point 2.3 where certain errors will bomb out the server, which is a little annoying, but if you've got fixes that's great, as long as all errors that were bombing it out are all now caught. I just have one line in your PR I'm curious about (return Promise.all([buildAssets(buildConfig), buildAdditionalAssets(buildConfig)])) - this doesn't appear to have a catch and log or a catch and process.exit on it? It's on line 44.

Agree with all other points. Would be nice to have the config as ts - webpack and others manage it, and there appear to be examples of others using .ts files for esbuild config files, but type annotations would be a great first step.

@ushkarev
Copy link
Contributor

i’ve made the first pass at the above in hmpps-template-typescript#389 and included your error-catching tweaks @Thomas-Geraghty

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.

5 participants