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

Switch from webpack to esbuild #19

Merged
merged 35 commits into from
Sep 14, 2024
Merged

Switch from webpack to esbuild #19

merged 35 commits into from
Sep 14, 2024

Conversation

denis-ok
Copy link
Collaborator

@denis-ok denis-ok commented Nov 3, 2023

redacted by @rusty-key

This PR removes webpack with esbuild for the sake of speed and simplicity.

How to test

Fetch the PR and run:

# build the project
$ make start_example

# serve resulting bundle
$ make serve_example

denis-ok and others added 7 commits August 2, 2024 15:15
* main:
  Update changelog
  Update melange constraint
  Remove pin for melange
  Remove reason-react pin, install a regular ocaml version
  melange v3: fixes for latest changes
  support melange v3
  update to latest rr
  update to reason-react 0.14
@jchavarri
Copy link
Member

Some things that we have to fix before merging this PR:

  • Esbuild live reload doesn't work when Dune runs reshowcase, because the esbuild devserver gets killed every time a file changes, which breaks the eventsource connection (see Add proxy support #32 (comment)). We should replace start-example in Makefile with two commands: one to build the example with Dune, and another to call reshowcase start.
  • The call to start-example gets passed --output which is not needed in that mode
  • Add changelog entry
  • Update README, which references webpack

@jchavarri jchavarri mentioned this pull request Sep 9, 2024
@rusty-key rusty-key changed the title Esbuild Switch from webpack to esbuild Sep 9, 2024
@rusty-key
Copy link
Collaborator

@jchavarri, I merged proxy support PR, detached dune/esbuild and updated readme/changelog. Please take another look

@rusty-key rusty-key marked this pull request as ready for review September 9, 2024 13:47
Copy link
Member

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor fixes and suggestions. Thanks!

CHANGES.md Outdated Show resolved Hide resolved
Makefile Outdated

.PHONY: serve-example
serve-example: ## Serves example on given port
$(BUILD_DIR)/commands/reshowcase start --entry=./$(BUILD_DIR)/example/example/example/Demo.js -port=8000
Copy link
Member

Choose a reason for hiding this comment

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

I think -port has two dashes:

Suggested change
$(BUILD_DIR)/commands/reshowcase start --entry=./$(BUILD_DIR)/example/example/example/Demo.js -port=8000
$(BUILD_DIR)/commands/reshowcase start --entry=./$(BUILD_DIR)/example/example/example/Demo.js --port=8000

(also why do we need to pass it if it's the same value as the default?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need to pass it, you are right. Removed

@@ -59,17 +59,6 @@ opam pin add reshowcase.dev git+https://github.com/ahrefs/reshowcase.git#main

This will make the NodeJS script `reshowcase` available in your opam switch.

To make sure this script works, add the following dependencies to your application `package.json`:
Copy link
Member

Choose a reason for hiding this comment

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

I'd mention here esbuild and the @craftamap/esbuild-plugin-html plugin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

added, but maybe it's better to vendor it?

Comment on lines +115 to +117
entryNames: "[name]-[hash]",
assetNames: "[name]-[hash]",
chunkNames: "[name]-[hash]",
Copy link
Member

Choose a reason for hiding this comment

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

These are needed because otherwise it would use the default [dir]/[name] right? Not sure if worth a comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually do not no, they were here before me. Will check

return console.error(`[reshowcase] ${error}`)
} else {
console.timeEnd(durationLabel);
console.log(`[reshowcase] http://localhost:${port}`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(`[reshowcase] http://localhost:${port}`)
console.log(`[reshowcase] Server started at http://localhost:${port}`)

@@ -84,6 +73,6 @@ $ reshowcase start --entry=path/to/Demo.js
$ reshowcase build --entry=path/to/Demo.js --output=path/to/bundle
```

If you need custom webpack options, create the `.reshowcase/config.js` and export the webpack config, plugins and modules will be merged.
If you need custom esbuild options, create the `.reshowcase/config.js` and export the esbuild config. Plugins and modules will be merged.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should mention here that if users use their own template, they have to add the new EventSource('/esbuild')... script manually if they want to keep livereload working.

Copy link
Collaborator Author

@denis-ok denis-ok left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for finishing!
Have one suggestion.

Comment on lines +238 to +243
const server = http.createServer((req, res) => {
let options = {
path: req.url,
method: req.method,
headers: req.headers,
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we add a minimal graceful shutdown for esbuild server and node http server?

We'll listen for SIGINT and SIGTERM signals.

For esbuild:

When you are done with a context object, you can call dispose() on the context to wait for existing builds to finish, stop watch and/or serve mode, and free up resources.

https://esbuild.github.io/api/#build

For node HTTP server:

Stops the server from accepting new connections and closes all connections connected to this server which are not sending a request or waiting for a response.

https://nodejs.org/api/http.html#serverclosecallback

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done: 592039e

$ make build && make serve-example
opam exec -- dune build
_build/default/commands/reshowcase start --entry=./_build/default/example/example/example/Demo.js
[reshowcase] Watch and serve started. Duration: 25.562ms
[reshowcase] http://localhost:8000
[esbuild] Rebuild finished!
^C[reshowcase] Received shutdown signal, shutting down gracefully...
[reshowcase] Server shut down gracefully

@rusty-key rusty-key merged commit d10ea8e into main Sep 14, 2024
@rusty-key rusty-key deleted the esbuild branch September 14, 2024 18:54
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