-
Notifications
You must be signed in to change notification settings - Fork 213
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
Add a Node platform. #663
Add a Node platform. #663
Conversation
Could we add more details to the commit message? |
Sure! What are you looking for in particular? |
Mainly hoping for a breakdown of what are changes to existing extension points to support this vs the pieces that would go in for any new platform. |
Done. |
This also makes CompilerPool more general, so it can be used by both the node and the browser platforms.
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, whew! :-)
Future<Pair<Process, StackTraceMapper>> _spawnProcess( | ||
String path, SuiteConfiguration suiteConfig) async { | ||
var dir = new Directory(_compiledDir).createTempSync('test_').path; | ||
var jsPath = p.join(dir, p.basename(path) + ".node_test.dart.js"); |
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.
nit: consistent use of single quote here with rest of file.
|
||
throw new LoadException( | ||
suitePath, | ||
"Error getting $url: ${response.statusCode} " |
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.
consider using single quotes here and below
} on IOException catch (error) { | ||
var message = getErrorMessage(error); | ||
if (error is SocketException) { | ||
message = "${error.osError.message} " |
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.
okay I'm going to stop posting about this quote thing, but check out other places in these files that it's happening.
I think the styleguide intentionally doesn't provide guidance on string quotes, because keeping them consistent is a lot of overhead for little benefit. This package certainly doesn't strive for consistency in general. I'd prefer to leave them as-is. |
No description provided.