Clean up some helpers #1117
Clean up some helpers #1117
Conversation
Address comments from #1109.
These were caught by a local run of eslint-config-standard.
There was a problem hiding this comment.
This looks good to me. However, the change from Object.create(null) to new Map is not doing what you expect - You need to use the Map's get and set methods. Doing map[foo] is setting a property on the map object, rather than actually setting a value in the map. lol javascript
| @@ -120,7 +120,7 @@ function sendTokenToAllServers(token) { | |||
| strictSSL: false, | |||
| }; | |||
| request(options, function(err, res, body) { | |||
| if (err != null) { return reject('Posting the GitHub user token failed: ' + err.stack); } | |||
| if (err != null) { return reject(err); } | |||
There was a problem hiding this comment.
Could do return reject(new Error('Posting the GitHub user token failed: ' + err.message)) or something similar. Up to you.
There was a problem hiding this comment.
Yea… I thought about that. This is called in a loop, and the calling code already has something really similar to that. It feels redundant to prefix it twice.
| return false; | ||
| } | ||
| const pattern = /(\d+\.)*\d-SNAPSHOT/; | ||
| return version && version.match(pattern); |
lib/regular-update.js
Outdated
| @@ -4,7 +4,7 @@ const request = require('request'); | |||
|
|
|||
| // Map from URL to { timestamp: last fetch time, interval: in milliseconds, | |||
| // data: data }. | |||
| let regularUpdateCache = Object.create(null); | |||
| let regularUpdateCache = new Map(); | |||
There was a problem hiding this comment.
All regularUpdateCache[url] need to be changed to either regularUpdateCache.get(url) or regularUpdateCache.set(url, value) in order to use Map
There was a problem hiding this comment.
We have no unit test or service test coverage on regularUpdate. I started to write unit tests but think the time is better spent integrating a library like https://github.com/ptarjan/node-cache or https://github.com/mpneuried/nodecache, which have their own tests, than maintaining this code.
There was a problem hiding this comment.
I'm going to reset these changes.
|
Haha, thanks for the education on |
Address comments from #1109.