search

gjaldon / heroku-buildpack-phoenix-static

A Heroku buildpack for building Phoenix's static assets
MIT License
229 stars 224 forks source link

Add remove_node option #36

Closed aaronjensen closed 7 years ago

aaronjensen commented 8 years ago

Fixes #35 Fixes #34

aaronjensen commented 8 years ago

Ok, I removed the MIX_ENV setting. Let me know if you want me to npm prune --production if remove_node is not true or change the default.

aaronjensen commented 8 years ago

@gjaldon is this good now?

aaronjensen commented 7 years ago

@gjaldon ping

gjaldon commented 7 years ago

@aaronjensen how 'bout doing npm prune --production to clean up and then check if there are any node modules left. If there are no node_modules left, then we remove Node. That way, we don't need to another config option. What do you think?

aaronjensen commented 7 years ago

@gjaldon that won't work for us as we have things in production and devDependencies to account for server side rendering that may or may not ever happen. We could move them all to devDependencies, sure, but I kind of like the distinction for now, especially since eslint supports that distinction (you can say only allow requires from your dependencies in your app directory, for example).

Furthermore, I think it's common for people to just put all of their stuff in dependencies even if they don't do anything server side. People also may be unknowingly depending on dev dependencies and doing what you propose would not be backwards compatible as it could break people.

I don't like configuration options any more than the next person, but in this case I think it's reasonable.

aaronjensen commented 7 years ago

@gjaldon any concern with me merging this in as-is? We can handle doing an npm prune --production by default or with its own config as a separate pull request if you're good w/ that.