Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Derbyjs Compatibility #31

Closed
wants to merge 2 commits into from

2 participants

@javaJake

Forms is almost completely compatible with Derbyjs. The provided patch allows forms to accept a derbyjs params array as input to its handle function.

I'm only a month into using NodeJS in general, so I'm all ears if you have concerns, however general.

One thing I noted is that you use instanceof and typeof in place of Object.prototype.toString.call, which is reportedly more reliable. The instanceof syntax, in particular, is limited in what it can check for, whereas Object.prototype.toString.call seems capable of handling almost all types.

@ljharb
Collaborator

instanceof is perfectly reliable with non-primitives, so it should remain for http.incomingMessage.

lib/forms.js
@@ -66,6 +66,9 @@ exports.create = function (fields) {
} else {
throw new Error('Cannot handle request method: ' + obj.method);
}
+ } else if (Object.prototype.toString.call(obj) === '[object Array]') {
@ljharb Collaborator
ljharb added a note

The proper way to check for an array is Array.isArray

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/forms.js
@@ -66,6 +66,9 @@ exports.create = function (fields) {
} else {
throw new Error('Cannot handle request method: ' + obj.method);
}
+ } else if (Object.prototype.toString.call(obj) === '[object Array]') {
+ // assuming derby params
+ f.handle(obj.body, callbacks);
} else if (typeof obj === 'object') {
@ljharb Collaborator
ljharb added a note

so derby passes an array that has a body parameter? that seems odd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@javaJake

The check for http.IncomingMessage should occur later, then, because instanceof causes an exception on arrays.

I admit I was confused when I saw it myself. The inspector said it was type array, with length 0, and had several attributes. I think it has to do with the way it was initialized before the attributes were applied. (Something like 'var route = {}' deep within derby if I recall correctly.)

Perhaps the best solution here is to have derbyjs pass obj.body to the handle function instead. I'll take another look.

@javaJake

Passing params.body directly allows me to get rid of that extra else-if logic. However, I was forced to keep the Object.prototype.toString.call to prevent the following exception:

Uncaught TypeError: Expecting a function in instanceof check, but got #<Object>

@ljharb
Collaborator

What version of node are you running? I can't believe there's a JS engine where [] instanceof Foo would fail as long as Foo exists. Perhaps in your version of node, http.IncomingMessage doesn't exist? If so, that's a different problem.

Also, please run tests against your patch with: NODE_PATH=pwd:pwd/deps/:pwd/lib/ node test.js

@javaJake

http.IncomingMessage exists because derby sits on top of express as middleware.

This is where I got the idea that instanceof is unreliable: http://stackoverflow.com/a/6021263

I'm running 0.8.7.

@ljharb
Collaborator

I can't accept this pull request at this time - it causes tests to fail, and when I console.log Object.prototype.toString.call(obj) for the cases where instanceof http.IncomingMessage would apply, I get "[object Object]".

Your issue may reflect an incompatibility with node 0.8, but I'm not going to risk breaking 0.6 without additional test cases.

@ljharb ljharb closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 1 deletion.
  1. +1 −1  lib/forms.js
View
2  lib/forms.js
@@ -46,7 +46,7 @@ exports.create = function (fields) {
handle: function (obj, callbacks) {
if (typeof obj === 'undefined' || obj === null || (typeof obj === 'object' && Object.keys(obj).length === 0)) {
(callbacks.empty || callbacks.other)(f);
- } else if (obj instanceof http.IncomingMessage) {
+ } else if (Object.prototype.toString.call(obj) === '[object IncomingMessage]') {
if (obj.method === 'GET') {
f.handle(parse(obj.url, 1).query, callbacks);
} else if (obj.method === 'POST' || obj.method === 'PUT') {
Something went wrong with that request. Please try again.