Merge pull request #2299 from mapbox/node-request-callback

[node] change request semantics

package.json

@@ -18,7 +18,7 @@
     }
   ],
   "dependencies": {
-    "nan": "nodejs/nan#a71301e0f62877ca017c4ebec7fbac9f4359038e",
+    "nan": "^2.0.9",
     "node-pre-gyp": "^0.6.9"
   },
   "bundledDependencies": [

platform/node/CHANGELOG.md

@@ -4,11 +4,13 @@
 - Requires an options object argument to `new mbgl.Map()`
   (with required `request` and optional `cancel` methods),
   drops `mbgl.FileSource`.
+- Changes `request` semantics to pass a second, callback argument instead
+  of needing to call `req.respond`.
 - Requires numerical `ratio` in `mbgl.Map` options argument.
   Map pixel ratio is now immutable and can no longer be set with
   render options.
 - Adds support for rendering v8 styles.
-- No longer load resources before a render request is made.
+- No longer loads resources before a render request is made.
 - Adds io.js v3.x support.
 
 # 1.1.3

platform/node/README.md

@@ -92,22 +92,22 @@ The `kind` is an enum and defined in [`mbgl.Resource`](https://github.com/mapbox
 
 It has no significance for anything but serves as a hint to your implemention as to what sort of resource to expect. E.g., your implementation could choose caching strategies based on the expected file type.
 
-THe `request` implementation should pass uncompressed data to `req.respond`. If you are downloading assets from a source that applies gzip transport encoding, the implementation must decompress the results before passing them on.
+The `request` implementation should pass uncompressed data to `callback`. If you are downloading assets from a source that applies gzip transport encoding, the implementation must decompress the results before passing them on.
 
 A sample implementation that reads files from disk would look like the following:
 
 ```js
 var map = new mbgl.Map({
-    request: function(req) {
+    request: function(req, callback) {
         fs.readFile(path.join('base/path', req.url), function(err, data) {
-            req.respond(err, { data: data });
+            callback(err, { data: data });
         });
     },
     ratio: 1.0
 });
 ```
 
-This is a very barebones implementation and you'll probably want a better implementation. E.g. it passes the url verbatim to the file system, but you'd want add some logic that normalizes `http` URLs. You'll notice that once your implementation has obtained the requested file, you have to deliver it to the requestee by calling `req.respond()`, which takes either an error object or `null` and an object with several settings:
+This is a very barebones implementation and you'll probably want a better implementation. E.g. it passes the url verbatim to the file system, but you'd want add some logic that normalizes `http` URLs. You'll notice that once your implementation has obtained the requested file, you have to deliver it to the requestee by calling `callback()`, which takes either an error object or `null` and an object with several settings:
 
 ```js
 {
@@ -125,14 +125,14 @@ var mbgl = require('mapbox-gl-native');
 var request = require('request');
 
 var map = new mbgl.Map({
-    request: function(req) {
+    request: function(req, callback) {
         request({
             url: req.url,
             encoding: null,
             gzip: true
         }, function (err, res, body) {
             if (err) {
-                req.respond(err);
+                callback(err);
             } else if (res.statusCode == 200) {
                 var response = {};
 
@@ -142,9 +142,9 @@ var map = new mbgl.Map({
                 
                 response.data = body;
                 
-                req.respond(null, response);
+                callback(null, response);
             } else {
-                req.respond(new Error(JSON.parse(body).message));
+                callback(new Error(JSON.parse(body).message));
             }
         });
     },
@@ -164,7 +164,7 @@ var request = require('request');
 var url = require('url');
 
 var map = new mbgl.Map({
-    request: function(req) {
+    request: function(req, callback) {
         var opts = {
             url: req.url,
             encoding: null,
@@ -177,7 +177,7 @@ var map = new mbgl.Map({
 
         request(opts, function (err, res, body) {
             if (err) {
-                req.respond(err);
+                callback(err);
             } else if (res.statusCode == 200) {
                 var response = {};
 
@@ -187,9 +187,9 @@ var map = new mbgl.Map({
             
                 response.data = body;
             
-                req.respond(null, response);
+                callback(null, response);
             } else {
-                req.respond(new Error(JSON.parse(body).message));
+                callback(new Error(JSON.parse(body).message));
             }
         });
     },

platform/node/src/node_file_source.cpp

@@ -75,11 +75,14 @@ void NodeFileSource::processAdd(const mbgl::Resource& resource) {
         queue->ref();
     }
 
-    v8::Local<v8::Object> requestHandle = NodeRequest::Create(this, resource)->ToObject();
+    auto requestHandle = NodeRequest::Create(this, resource)->ToObject();
     pending.emplace(resource, requestHandle);
 
-    v8::Local<v8::Value> argv[] = { requestHandle };
-    Nan::MakeCallback(Nan::New(options), "request", 1, argv);
+    auto callback = Nan::GetFunction(Nan::New<v8::FunctionTemplate>(NodeRequest::Respond, requestHandle)).ToLocalChecked();
+    callback->SetName(Nan::New("respond").ToLocalChecked());
+
+    v8::Local<v8::Value> argv[] = { requestHandle, callback };
+    Nan::MakeCallback(Nan::New(options), "request", 2, argv);
 }
 
 void NodeFileSource::processCancel(const mbgl::Resource& resource) {

platform/node/src/node_request.cpp

@@ -19,8 +19,6 @@ NAN_MODULE_INIT(NodeRequest::Init) {
     tpl->InstanceTemplate()->SetInternalFieldCount(1);
     tpl->SetClassName(Nan::New("Request").ToLocalChecked());
 
-    Nan::SetPrototypeMethod(tpl, "respond", Respond);
-
     constructor.Reset(tpl->GetFunction());
     Nan::Set(target, Nan::New("Request").ToLocalChecked(), tpl->GetFunction());
 }
@@ -55,7 +53,7 @@ v8::Handle<v8::Object> NodeRequest::Create(NodeFileSource* source, const mbgl::R
 }
 
 NAN_METHOD(NodeRequest::Respond) {
-    auto nodeRequest = Nan::ObjectWrap::Unwrap<NodeRequest>(info.Holder());
+    auto nodeRequest = Nan::ObjectWrap::Unwrap<NodeRequest>(info.Data().As<v8::Object>());
 
     // Request has already been responded to, or was canceled, fail silently.
     if (!nodeRequest->resource) {

platform/node/test/js/map.test.js

@@ -147,9 +147,9 @@ test('Map', function(t) {
 
     t.test('.render', function(t) {
         var options = {
-            request: function(req) {
+            request: function(req, callback) {
                 fs.readFile(path.join(__dirname, '..', req.url), function(err, data) {
-                    req.respond(err, { data: data });
+                    callback(err, { data: data });
                 });
             },
             ratio: 1

platform/node/test/render.test.js

@@ -13,9 +13,9 @@ if (process.argv[1] === __filename && process.argv.length > 2) {
 suite.run('native', {tests: tests}, function (style, options, callback) {
     var map = new mbgl.Map({
         ratio: options.pixelRatio,
-        request: function (req) {
+        request: function(req, callback) {
             request(req.url, {encoding: null}, function (err, response, body) {
-                req.respond(err, {data: body});
+                callback(err, {data: body});
             });
         }
     });