Skip to content

Conversation

@nikhilkh
Copy link
Contributor

@nikhilkh nikhilkh commented Apr 7, 2016

When we release platforms it takes a day for people to be able to use them. This is sub-optimal and removing the timeout to keep the cache fresh makes sense.

// Returns a promise that resolves to directory containing the package.
function npm_cache_add(pkg) {
var npm_cache_dir = path.join(util.libDirectory, 'npm_cache');
// 'cache-min' is the time in seconds npm considers the files fresh and
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider totally removing calls to 'npm cache add'. Also, are we sure that npm doesn't set some kind of default cache-min ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How will it work if I remove the caching altogether - sounds like a design change.

It does get a default cache-min of 10 seconds which if it is fine for npm, should be fine for us:
https://github.com/npm/npm/blob/2a5977e0c65b244e92d848fcd56f2f80ba8cdf3b/lib/config/defaults.js#L125. Their default scenario is quite similar to ours.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good.

@purplecabbage
Copy link
Contributor

I think Steve is addressing this in his changes to fetch.
Cc: @stevengill

@nikhilkh
Copy link
Contributor Author

nikhilkh commented Apr 8, 2016

Yes, I'm aware he is removing npm cache completely. This fix is still valuable in the interim till Steve completes the fetch work.

@omefire
Copy link
Contributor

omefire commented Apr 11, 2016

LGTM!
I also think we can merge this while waiting for Steve's changes.
If no one objects, I'll go ahead with the merge.

@purplecabbage
Copy link
Contributor

LGTM!

@asfgit asfgit closed this in d8cd62b Apr 11, 2016
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.

3 participants