-
Notifications
You must be signed in to change notification settings - Fork 6
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
Prepare the library to be released as a npm package #47
Conversation
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.
Looks good, but I do have a few minor questions and suggestions.
"irc": "irc://irc.freenode.net/wikidata" | ||
}, | ||
"require": { | ||
"data-values/javascript": "~0.8.0|~0.7.0", |
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.
Uh, we never made it compatible with DataValues JavaScript 0.9?
@@ -1,13 +1,48 @@ | |||
{ | |||
"name": "wikibase-serialization", |
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.
Again, tab-indention should be the norm for .json files. But this is for later.
@@ -1,4 +1,5 @@ | |||
/** | |||
* @ignore | |||
*/ | |||
window.wikibase = window.wikibase || {}; |
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.
Why does this use window.
, but the line below does not? In my opinion this a) should be consistent, and b) avoid window.
when not really necessary (e.g. certain headless JS engines might need it).
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.
It was needed in this line exactly to have phantom js understand where wikibase object is defined.
It was not apparently needed elsewhere. The whole weirdness is probably going to be irrelevant once this lib uses some proper way of registering its stuff (module.exports or whatever you do in JS this month).
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.
Ok, thanks!
Requires releases of dependency libraries as npm packages (see: wmde/DataValuesJavaScript#129 and wmde/WikibaseDataModelJavaScript#82).
Includes a commit running Qunit tests using npm test initially submitted as #45.