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

Halogen dev workflow, with esbuild #1085

Merged
merged 29 commits into from
Nov 15, 2023
Merged

Halogen dev workflow, with esbuild #1085

merged 29 commits into from
Nov 15, 2023

Conversation

CharlesTaylor7
Copy link
Collaborator

@f-f Alternative to #1072, based on your feedback: #1072 (comment)

This version avoid pulling in vite, and just bundles with ESbuild in watch mode.
Overall, it's probably less maintenance overhead, but you will have to manually refresh the page index.html file after the bundling completes.

package.json Outdated
@@ -30,7 +30,8 @@
"format": "purs-tidy format-in-place core src bin docs-search",
"format:check": "purs-tidy check core src bin docs-search",
"bundle": "spago bundle -p spago-bin",
"prepublishOnly": "spago build && ./bin/index.dev.js bundle -p spago-bin && ./bin/index.dev.js bundle -p docs-search-client-halogen"
"prepublishOnly": "spago build && ./bin/index.dev.js bundle -p spago-bin && ./bin/index.dev.js bundle -p docs-search-client-halogen",
"halogen:dev": "echo \"#!/usr/bin/env node\n\nimport { main } from './output/Docs.Search.App/index.js'; main();\" | esbuild --platform=browser --format=iife --sourcemap --bundle --outfile=generated-docs/html/docs-search-app.js --loader:.node=file --alias:punycode=punycode/ --watch=forever"
Copy link
Collaborator Author

@CharlesTaylor7 CharlesTaylor7 Oct 14, 2023

Choose a reason for hiding this comment

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

A few notes:

  • spago bundle only allows extraArgs in the spago.yaml configs, not from the command line. So I was forced to use esbuild directly.
  • Esbuild doesn't know its supposed to invoke the main function so I copied the same workarounds that Spago.Command.Bundle.purs uses. i.e. it needs the --iife flag, and the main function to be invoked via extra stdin input.
  • --watch=forever is so that it keeps watching even after stdin stops receiving input. This does the right thing ™️ , it rebuilds when output/ dependencies change. If you run in regular watch mode, you get this error:
[watch] build finished, watching for changes...
[watch] stopped because stdin was closed (use "--watch=forever" to keep watching even after stdin is closed)

Copy link
Member

Choose a reason for hiding this comment

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

only allows extraArgs in the spago.yaml configs, not from the command line

Oh, that looks like an oversight 😄 I think we have a good argument to add this now

Copy link
Contributor

Choose a reason for hiding this comment

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

it needs the --iife flag

Why does it need that?

Copy link
Member

Choose a reason for hiding this comment

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

Why does it need that?

To bundle for the browser, see esbuild docs

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Wasn't there some other reason though about why we couldn't use ESM specifically? I thought there was some library that had an issue, but I can't recall which or why.

@f-f
Copy link
Member

f-f commented Oct 14, 2023

Windows CI doesn't look happy, but I like the overall looks of this much better. I think we can fix the bundle extraArgs here, so we can use a bundle command instead of directly calling esbuild

@CharlesTaylor7
Copy link
Collaborator Author

CharlesTaylor7 commented Oct 17, 2023

@f-f On Windows, where does spago bundle output go?

I inserted a sanity checking step of
ls bin right after bundling, and the CI is saying that neither bin/bundle.js nor bin/docs-search-app.js is present even right after calling the bundle commands:

https://github.com/purescript/spago/actions/runs/6541347144/job/17762743250?pr=1085#step:13:2

@f-f
Copy link
Member

f-f commented Oct 17, 2023

Yeah, that's very confusing - I'll have a look

@f-f
Copy link
Member

f-f commented Oct 17, 2023

I can't push to your branch (for future PRs you could make branches in this repo so we can all push to them) but I would try to ls the current folder as well

@CharlesTaylor7
Copy link
Collaborator Author

CharlesTaylor7 commented Nov 11, 2023

@f-f Hey, I'm back and trying to debug this.

I found something weird. On Windows only, If you look at the last two workflow runs, you can see the bundle step works when using the globally installed spago package, but does nothing when using the freshly built ./bin/index.dev.js.

Both are bundling in verbose mode, so there should be lots of console output:

✅ Working:
https://github.com/purescript/spago/actions/runs/6834906185/job/18588114472?pr=1085#step:12:1

❌ Failing with very little console output:
https://github.com/purescript/spago/actions/runs/6834832975/job/18587969681#step:12:1

@f-f
Copy link
Member

f-f commented Nov 13, 2023

you can see the bundle step works when using the globally installed spago package, but does nothing when using the freshly built ./bin/index.dev.js

Oh! Of course. Calling an executable to be run with ./some-program will use the shebang definition on Linux/macOS (which we do include when bundling), but does not work on Windows. We should switch that call from ./bin/index.dev.js to node ./bin/index.dev.js (which will work cross-platform)

bin/src/Flags.purs Outdated Show resolved Hide resolved
bin/src/Flags.purs Outdated Show resolved Hide resolved
bin/src/Main.purs Outdated Show resolved Hide resolved
@f-f
Copy link
Member

f-f commented Nov 15, 2023

This looks great now - thanks @CharlesTaylor7! 👏👏

@f-f f-f merged commit 6a33fe3 into purescript:master Nov 15, 2023
3 checks passed
@CharlesTaylor7 CharlesTaylor7 deleted the esbuild-dev-workflow branch November 16, 2023 03:11
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