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

Make brunch easier to replace #1540

Closed
rstacruz opened this Issue Feb 11, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@rstacruz
Contributor

rstacruz commented Feb 11, 2016

Brunch is currently hooked onto Phoenix apps by way of dev.exs, where:

config :app, App.Endpoint,
  watchers: [node: ["node_modules/brunch/bin/brunch", "watch", "--stdin"]]

As well as brunch build as part of many deploy scripts.

IMHO, a saner default is to outsource these commands to npm hooks, where:

// package.json
{
  "scripts": {
    "build": "brunch b",
    "watch": "brunch w --stdin"
  }
}

They can be ran via:

config :app, App.Endpoint,
  watchers: [node: ["npm", "run", "watch"]]

This way, it can easily be swapped for grunt/gulp/webpack/browserify/anything else. These can be easily hooked onto any Phoenix app at the moment, but I'm proposing they be done by default in the mix phoenix.new generator and promoted in the docs. I'd be happy to help issue PR's for this.

@Gazler

This comment has been minimized.

Member

Gazler commented Feb 11, 2016

I like this idea. Small correction in your description, it should be:

config :app, App.Endpoint,
  watchers: [npm: ["run", "watch"]]

The would certainly prevent the need for overriding the compile script in the phoenix static buildpack if using something other than brunch.

@josevalim

This comment has been minimized.

Member

josevalim commented Feb 11, 2016

Hey @rstacruz! Wasn't it you that designed the exception page we use in Phoenix? :)

I believe we tried this in the past in regards to the watch command but, iirc, we ran into issues with Windows. It would be worth to find the previous issue and investigate once again.

In any case, 👍 to at least add a deploy command to the package.json file so we can at least make sure deployment scripts are not brunch dependent. The watch command is a minor issue because it is just a development local option anyway.

@rstacruz

This comment has been minimized.

Contributor

rstacruz commented Feb 11, 2016

Hey @rstacruz! Wasn't it you that designed the exception page we use in Phoenix? :)

i was surprised first first time i saw that then I got all giddy, haha. what was the issue with Windows? npm not being executable?

@josevalim

This comment has been minimized.

Member

josevalim commented Feb 11, 2016

See #1322. We don't say exactly which issues they were. So we would need to try it out.

@josevalim josevalim closed this in b932f26 Feb 16, 2016

@josevalim

This comment has been minimized.

Member

josevalim commented Feb 16, 2016

@rstacruz I have added both scripts but we can't change the config/dev.exs file until some kind soul battle test these things on Windows. We hope this at least solves the deploy issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment