-
Notifications
You must be signed in to change notification settings - Fork 47
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
Basic source map support #27
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.
LGTM. I was just curious about some areas and left comments, but not blocking 🙂
@@ -43,10 +44,15 @@ export class App { | |||
return this.releaseInfo.source_hash | |||
} | |||
|
|||
get sourceMap() { | |||
return this.releaseInfo.source_map |
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.
Is there a convention for snake case vs camel case (I'm genuinely curious)? do we only use snake case for JSON going to web app?
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.
Yes, Ruby's convention is snake_case and since that's what the API is written in, that's the data we receive and need to send.
At some point we should figure out a way to homogenize this. I've never knew what to do in these cases, so I'm just following each language's conventions.
@@ -5,7 +5,7 @@ | |||
"main": "lib/index.js", | |||
"scripts": { | |||
"start": "./bin/fly server ./apps/getting-started", | |||
"test": "mocha", | |||
"test": "node ./lib/cmd/index.js test v8env_test/**/*.spec.js && mocha", |
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.
smartz (using fly test
)!
}) | ||
|
||
setImmediate(() => |
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.
not sure what the implication is of changing setImmediate
only once request is finalized?
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's a readability change. We used to attach the event handlers in the setImmediate
also. It just wasn't necessary and made it harder to reason about.
We need that setImmediate
only for when we start reading or writing to the body.
Pull Request Test Coverage Report for Build 243
💛 - Coveralls |
fly test