-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
cache client/node_modules during heroku deploys #324
Conversation
@modosc This looks great. Can you please add a couple sentences to the heroku doc page. Also, maybe this is more generic than heroku? |
Additionally, we have to see why the CI build does not pass with this. |
seems like it failed for an unrelated reason:
i can't seem to retrigger the build tho. |
@modosc any word on this one? |
updated docs, let's see if ci is happy today. |
@modosc Please rebase on top of master to a single commit. Thanks. |
@@ -29,5 +29,6 @@ | |||
"url": "https://github.com/shakacode/react-webpack-rails-tutorial/issues" | |||
}, | |||
"homepage": "https://github.com/shakacode/react-webpack-rails-tutorial", | |||
"dependencies": {} | |||
"dependencies": {}, | |||
"cacheDirectories": ["node_modules", "client/node_modules"] |
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.
@modosc there should be no node_modules directory, right? Just client/node_modules.
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.
it's possible the app has one for some other reason (separate service, browserify
integration, etc) and it's the heroku default if nothing else set so i figured it made sense to keep it just in case.
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.
OK
@modosc can you please rebase on top of master. I want to merge this. |
@modosc I'm ready to merge your changes. However, we need to rebase on top of master, after squashing to one commit, and please include the changelog entry. |
@modosc Ping, Ping. I'm ready to merge your changes. However, we need to rebase on top of master, after squashing to one commit, and please include the changelog entry. |
without this heroku has to do a full npm install on every deploy - i guess this could be part of the heroku generators but it seemed like a lot more work to modify this file there than to just add this option here.
@justin808 should be done? |
Manually merged in #415. |
without this heroku has to do a full npm install on every deploy - i guess this could be part of the heroku generators but it seemed like a lot more work to modify this file there than to just add this option here.
This change is