-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added server-side node compatibility #123
Conversation
…le requires, url parsing, node only errors
…added node shim for XMLHttpRequest, fixed a scope issue
Tracking, but a PR of this scale is going to take a bit to move forward. While I get the interest in having an NPM module with xAPI-everything included, it feels like this would make curation pretty muddy. Will review this in the next week or so. |
Let me know once you've reviewed it. |
@@ -1,3 +1,4 @@ | |||
(function(ADL, root){ | |||
// adds toISOString to date objects if not there | |||
// from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString |
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.
I've brought all of the functions at the top of this file into the closure defining this module. This is intentional, there doesn't seem to be a good reason to leak these functions into the global scope.
I would add that shimming the native Date object isn't a good idea as this library is clearly intended for use in other, potentially conflicting systems.
Changing it to the following might be more appropriate:
function DateToISOString(date) {
}
* @param {function} [complete ] completion callback function | ||
* @return {object} xhr response object | ||
*/ | ||
function http(method, url, headers, data, options, complete) |
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.
I'm not entirely sure about how I've defined of the http
function parameters, but the intent of the code (as a method of performing cross domain http request requests in different environments) seems easier to read when isolated like this. The http
function should hopefully exhibit identical behaviour to the original code which was nested inside the XHR_request
function, as it's almost a verbatim copy. I have left the LRS specific http request modifications in the XHR_request
function to create a distinct line between LRS stuff and the http request stuff. I hope that makes sense? This bit will need the most testing.
|
||
// synchronous call in IE, with no asynchronous mode available. | ||
if (!async && ieXDomain) { | ||
var until = 1000 + new Date(); |
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.
At some point it might be worth dropping synchronous calls entirely?
body = xhr.responseText; | ||
} | ||
var result; | ||
http( |
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 is where the new http
function is used, returning the xhr
object as produced.
It greatly simplifies the XHR_request
function to abstract away the xhr
production but it isn't necessary that this change happen for the node XMLHttpRequest
shim to work.
onBrowser = true; | ||
} | ||
else { | ||
ADL = require('../../dist/xapiwrapper.min'); |
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.
If this test is run in node, this is where the new umd wrapped xapiwrapper is loaded into node for testing.
I have taken the javascript from the example.html
, swapped the request from synchronous to asynchronous and added as the first real test of the api in node. Other tests would need to follow.
// Browser roots | ||
factory({}, root, root); | ||
} | ||
}(typeof self !== 'undefined' ? self : this, function (module, window, root) {//<%= output => |
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 is a slightly modified umd wrapper to overcome an issue associated with bundling the text-encoding
library with the xapiwrapper.min.js
. Essentially the text-encoding
code tried to performmodule.exports
which overrides the export of the xapiwrapper.min.js
. In order to stop that I've redefined an empty module
variable inside the closure.
In both requirejs
and node
environments, the ADL
api object is now returned as the module export. Otherwise the ADL
api is added to the window
or global scope as before. It might be worth dropping the requirejs
support as I can imagine this library is already used in mixed global scope + requirejs environments and this change to support requirejs
would remove the ADL
api from the global scope.
|
||
if (isNode) { | ||
// Import encoding-indexes as required by text-encoding module | ||
var eIndexes = require('../node_modules/text-encoding/lib/encoding-indexes'); |
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.
In node, because the text-encoding/lib/encoding.js
file has been bundled into the xapiwrapper.min
file encoding.js
has lost its original relative path. In node the bundled file requires the encoding-indexes
hash, which is not needed in the browser. These lines import the encoding-indexes
file into the global scope as they would be if the encoding.js
file were in its original location. It isn't great but it does work. An alternative would be to bundle both files but as encoding-indexes
is not needed in the browser it would bloat the size of the library for that environment.
The text-encoding
library provides a shim for the TextDecoder
and TextEncoder
apis.
} | ||
|
||
// Node shim for browser location | ||
var location = isNode ? |
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.
The location object is used in the browser to determine a few things about how the http request should be made. It doesn't exist in node so this is just the minimal necessary replacement to get the code working.
It would be better just to skip the location checks in node altogether.
@@ -184,7 +182,7 @@ function isDate(date) { | |||
|
|||
if (verifyxapiversion && testConfig.call(this)) | |||
{ | |||
window.ADL.XHR_request(this.lrs, this.lrs.endpoint+"about", "GET", null, null, | |||
ADL.XHR_request(this.lrs, this.lrs.endpoint+"about", "GET", null, null, |
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.
I have removed all module internal references to the global scope window
as it's different in different environments. The umd wrapper normalises all of this and it's unnecessary anyway as we already have a reference to the ADL object.
@@ -165,8 +164,7 @@ function isDate(date) { | |||
|
|||
function getbase(url) | |||
{ | |||
var l = document.createElement("a"); | |||
l.href = url; | |||
var l = parseUrl(url); |
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.
The document
object doesn't exist in node so I've added a cross platform parseUrl
function to appropriately differentiate. The parseUrl
function returns a subtly different parsing object in both environments, but the differences don't matter much here.
Just checking in. Any thoughts on this? |
Ok, going to look at this today. Do you have a sample |
I was using the tests to do that. This file can be run using This one to get the above text running in the browser: It demonstrates the xapiwrapper.min.js statement, verbs, activity etc api working in both node and the browser. Launch doesn't work in node. It'll give an error. |
Simple examples seem to run, will look more at it later. 👍 In the meantime, it doesn't produce intellisense and we'll probably need to have more rigorous tests with more complex statement varieties. |
Yup, totally agree. Do you have any suggestions for statement varieties? For the intellisense I reckon it'd be easiest to produce a non-minified but compiled version with comments intact. I will have a look to see if we can just import the uncompiled source files instead though. Irritatingly the ADL object transcends multiple files but hopefully the code is segregated enough to allow that to happen without one file needing references to code in another. I don't know if it's preferable to use the uncompiled source directly? Any thoughts? |
Closed in favour of #126 : with intellisense and a few of the more complex bits removed. |
#67
Works
Notes
xapiwrapper.min.js
const adl = require('xapiwrapper');
if published on npm.ADL.launch
in node