-
Notifications
You must be signed in to change notification settings - Fork 10
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
JSTP 0.6.3 proposal #39
Conversation
* Enable build on Windows using the native extensions if the build succeeded or falling back to JavaScript implementation otherwise. Spawn node-gyp under shell so that $PATH resolves correctly on Windows. * Remove check for Node.js versions < 4.0 since Node.js 6.0 is now the oldest supported version.
This commit adds explicit type casts to resolve compiler warnings about possible data loss.
Rename Object Serialization example from `person.jsrs` into `person.js` and add a missing Record Serialization example under that name.
The last column of the implementations table had been removed before but not in the columns definition row. GitHub's markdown parser has been able to parse it so it went unnoticed however the parsed used by docs generator inserts an empty column in such case.
Remove `const server = this;` lines in `lib/server.js` which were left from times when arrow functions were not used.
Remove the unnecessary "All rights reserved" phrase since the last country which laws used to required it got rid of it in 2000 and it has meant nothing throughout the rest of the world since very long time ago.
* Replace license text with references to the LICENSE files. * Update years.
* Split `jsrs-impl.cc` into separate modules. * Make some refactoring. * Rename the native addon to `jstp` since there already is a function that is not a part of JSRS. * Fix `binding.gyp`: make `cflags` not ignored on macOS (as it appeared they used to be) and do not use `-O3` in Debug configuration. * Use a macro to throw V8 exceptions to avoid boilerplate code. PR-URL: #36
* Use `cflags_cc` instead of `cflags`. * By default `node-gyp` uses `-std=gnu++0x`. This commit modifies `cflags_cc` to use `-std=c++11`. PR-URL: #37
* Update `karma-webpack`. * Enable native addon build on Windows (falling back to JavaScript implementation if the toolset is not installed). * Internal changes. PR-URL: #39
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.
LGTM
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.
Comments are not critical
build-native.js
Outdated
@@ -27,5 +19,9 @@ nodeGyp.on('exit', (code) => { | |||
if (errorLines.length > 0) { | |||
fs.writeFileSync('builderror.log', errorLines.join('\n')); | |||
} | |||
process.exit(code); | |||
if (code !== 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.
if (code)
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.
See #40
lib/record-serialization.js
Outdated
|
||
try { | ||
jsrsNative = require('../build/Release/jsrs'); | ||
jstpNative = require('../build/Release/jstp'); |
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.
To avoid enclosure try
, better create safe require
const safeRequire = name => {
try {
return require(name);
} catch (e) {
return e;
}
};
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.
* Handle the situation when a compiler cannot be spawned and `exit` event is not fired. * Make `0` a named constant. PR-URL: #40
Get rid of nested try-catch statements. Decouple `safeRequire` function that can be used later. PR-URL: #41
* Update `karma-webpack`. * Enable native addon build on Windows (falling back to JavaScript implementation if the toolset is not installed). * Internal changes. PR-URL: #39
@tshemsedinov @belochub commits that resolve the nits mentioned before are added to the release too. |
@aqrln, yes, I've already reviewed corresponding PRs, and including them in this PR seems fine to me. |
@belochub waiting for CI to complete ;) |
@tshemsedinov @belochub version 0.6.3 successfully published to NPM 🎉 |
Version 0.6.3
karma-webpack
.