-
Notifications
You must be signed in to change notification settings - Fork 582
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
feat: Bundle UI into coderd; add ./develop.sh script #28
Conversation
site/embed_static.go
Outdated
// The `embed` package ignores recursively including directories | ||
// that prefix with `_`. Wildcarding nested is janky, but seems to | ||
// work quite well for edge-cases. | ||
//go:embed out/_next/*/*/*/* | ||
//go:embed out/_next/*/*/* | ||
//go:embed out | ||
var site embed.FS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylecarbs - are you OK with using this same strategy for v2?
This is brought in mostly as-is from v1's embed_static.go
, except for removing unnecessary CSP entries for sentry/intercom which aren't integrated yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we create a single .gitkeep
in out/_next/static/chunks/pages/app
. Then we don't need to do the build flag!
Apart from that, this looks great!
Codecov Report
@@ Coverage Diff @@
## main #28 +/- ##
==========================================
- Coverage 72.00% 71.60% -0.41%
==========================================
Files 37 38 +1
Lines 1304 1486 +182
Branches 7 7
==========================================
+ Hits 939 1064 +125
- Misses 293 340 +47
- Partials 72 82 +10
Continue to review full report at Codecov.
|
site/embed_static.go
Outdated
// The `embed` package ignores recursively including directories | ||
// that prefix with `_`. Wildcarding nested is janky, but seems to | ||
// work quite well for edge-cases. | ||
//go:embed out/_next/*/*/*/* | ||
//go:embed out/_next/*/*/* | ||
//go:embed out | ||
var site embed.FS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we create a single .gitkeep
in out/_next/static/chunks/pages/app
. Then we don't need to do the build flag!
Apart from that, this looks great!
devbin/v2-dev
Outdated
# Do initial build - a dev build for coderd. | ||
# It's OK that we don't build the front-end before - because the front-end | ||
# assets are handled by the `yarn dev` devserver. | ||
make bin/coderd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant to promote the devbin
workflow in v2 until we need it. It became really unmaintainable in v1 due to poor scoping, so I'd prefer to punt this as long as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just have CODERV2_HOST
default to :3000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty useful for developing the front-end - because you get a hot-reload on top of a live instance. I can just keep it local for now, though.
The main thing I want is to have a simple workflow for front-end developers jumping in - would your expectation be that they just run make bin/coderd
; bin/coderd
; and then open up yarn dev
in a separate terminal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed up the change to default the port to 3000 here: d9c6aeb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I commented on this, but I guess not!
Thoughts on having develop.sh
at the root instead? It's required to contribute anyways, and then is very obvious to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great to me! Moved devbin/v2-dev
-> develop.sh
in 237f83e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor nits!
.gitignore
Outdated
# Build | ||
bin/ | ||
out/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be site/out/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch - indeed it should: 65d0145
Makefile
Outdated
build: site/out bin/coderd | ||
.PHONY: build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep these sorted in alphabetical order. I'll see if there's a Makefile linter we can use!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the change locally for now in 8902599
devbin/v2-dev
Outdated
# Do initial build - a dev build for coderd. | ||
# It's OK that we don't build the front-end before - because the front-end | ||
# assets are handled by the `yarn dev` devserver. | ||
make bin/coderd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I commented on this, but I guess not!
Thoughts on having develop.sh
at the root instead? It's required to contribute anyways, and then is very obvious to users.
This change bundles the static assets like we have for v1 - using the
embed
go package. Fixes #22In addition, it sets up a development script that runs
coderd
locally and serves the front-end, with hot-reloading. The script used isv2-dev
, which is in adevbin
folder:Summary of changes:
go
in theMakefile
embed
tagembed
tag - so we don't need to build the front-end twicenext export
build step to output front-end artifacts inout
site
package forgo
embed_static.go
andembed.go
. This is mostly brought in as-is from v1, except removing some intercom/sentry CSP entries that we aren't using.v2-dev
script, that runscoderd
and thenext
dev server side-by-sidesite
package as the fallback handler..gitignore
entries for additional build collateral