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

Cache node_modules to speed up CI #4484

Merged
merged 1 commit into from Aug 15, 2018
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 6, 2018

The CI process is a little slow, even sometimes it fails because of timeout downloading node packages. A downside of this change: we need to remember to clear the cache if there is a change in js dependencies (which is not so common).

@stsewd stsewd requested a review from Aug 6, 2018
humitos
humitos approved these changes Aug 6, 2018
Copy link
Member

@humitos humitos left a comment

I'm 👍 with this change.

I prefer to speed up things in the day to day, and fail and clean when a node module has changed.

Loading

Copy link
Member

@ericholscher ericholscher left a comment

👍 on faster builds. I don't edit the node stuff much, but I wonder if there is a way to detect when it has changed and not cache it, or destroy the cache?

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 8, 2018

I wonder if there is a way to detect when it has changed and not cache it, or destroy the cache?

I think we only need to remind ourselves to clean the cache on travis, I'm not sure if travis clean the cache regularly, also, we don't have tests on the client side... So, no big problem I think

Loading

@humitos
Copy link
Member

@humitos humitos commented Aug 8, 2018

In fact, I think that we only need npm for eslint. So, I suppose that we will only note this in case we update our package.json for any eslint related package.

Loading

.travis.yml Outdated
@@ -20,6 +20,7 @@ cache:
directories:
- ~/.cache/pip
- ~/.nvm/nvm.sh
- node_modules
Copy link
Member

@safwanrahman safwanrahman Aug 8, 2018

Choose a reason for hiding this comment

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

Its not good idea to cache the node_modules, so if we remove any of the dependencies, it will be there.
Better cache the npm cache directory, so npm does not do network request for the cached package.

Loading

Copy link
Member Author

@stsewd stsewd Aug 10, 2018

Choose a reason for hiding this comment

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

Loading

Copy link
Member

@safwanrahman safwanrahman left a comment

Requested change for caching npm cache directory

Loading

@stsewd stsewd force-pushed the cache-node-modules branch from 1a5e34d to 70d0867 Aug 10, 2018
Copy link
Member

@humitos humitos left a comment

👍

Loading

@humitos humitos merged commit ddf1ef8 into readthedocs:master Aug 15, 2018
1 check passed
Loading
@stsewd stsewd deleted the cache-node-modules branch Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants