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

Split dev assets and prod assets #99

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

hhubert6
Copy link
Contributor

No description provided.

@hhubert6 hhubert6 requested a review from kraleppa February 20, 2025 12:13
Copy link
Member

@kraleppa kraleppa left a comment

Choose a reason for hiding this comment

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

Also, sometimes developer wants to use mix assets.build on the dev mode (the same command which Github bot uses for building assets).

I'd be good to parematrize this alias so we have e.g. mix assets.build dev or mix assets.build deploy

This will have to be changed in README too (see contributing section)

.gitignore Outdated
@@ -31,3 +31,4 @@ live_debugger-*.tar

node_modules/

/priv/static/dev/
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
/priv/static/dev/
/priv/static/dev/

@@ -11,6 +11,12 @@ if config_env() == :dev do
~w(js/app.js --bundle --minify --sourcemap=external --target=es2020 --outdir=../priv/static/),
cd: Path.expand("../assets", __DIR__),
env: %{"NODE_PATH" => Path.expand("../deps", __DIR__)}
],
dev: [
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 change naming to e.g. deploy_build and dev_build to emphasize that it is executed by bot after merge

@@ -23,6 +29,15 @@ if config_env() == :dev do
--minify
),
cd: Path.expand("../assets", __DIR__)
],
live_debugger_dev: [
Copy link
Member

Choose a reason for hiding this comment

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

same here: dev_build and deploy_build

Comment on lines 18 to 20
if Mix.env() == :dev do
plug(Plug.Static, at: "/assets", from: {:live_debugger, "priv/static/dev"}, gzip: false)
end
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 add else here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean to move plug(Plug.Static, at: "/assets", from: :live_debugger, gzip: false) to else? Because then priv/static/client.js won't be served in dev and I don't have better idea how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I forgot about client.js - it should be served both in dev and prod.

Copy link
Member

Choose a reason for hiding this comment

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

I know that it wasn't a scope of this task, but it'd be cool if you moved client.js to the assets folder and perform some kind of bundling on it to.

The current problem is that we currently serve client.js as it is - ideally we'd like a similar process for it as we have for assets/app.js file.

@hhubert6 hhubert6 requested a review from kraleppa February 21, 2025 09:44
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.

2 participants