-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
platform/node/index.js
Outdated
// Protect ourselves from `request` implementations that try to release Zalgo. | ||
// http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony | ||
try { | ||
request(req, function() { |
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.
Wrapping an async function in a try/catch always set off a🚨 in my head. Which function in here can throw?
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.
Nothing should throw. This is the API defending itself against badly behaved request
functions that do throw -- better to be defensive than crash in this situation.
var map = new mbgl.Map({ | ||
request: function() { | ||
throw new Error('message'); | ||
} |
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.
Can we add a test that first calls the callback, and then throws? This should ensure that the callback doesn't get invoked twice. Maybe also a test that has two invocations of callback
.
}); | ||
map.load(mockfs.style_vector); | ||
map.render({ zoom: 16 }, function(err, pixels) { | ||
t.error(err); |
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.
This test passes even without the code changes. Can we add a check to make sure the render function is called only once ?
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.
lastError
state when appropriate -- this was causing spurious errors when reusing aMap
object for multiple renders.