PoC: support babel-register #837
Conversation
googlebot
added
the
cla: yes
There was a problem hiding this comment.
Hey @mctavish thanks for opening up this PR, I think this is the best route; rather than completely forking the library, and I'm open to releasing to an experimental tag, e.g.,
npm i @google-cloud/debug@babel-register-poc
I'll make an effort to provide some review next week, it is probably also worth looping in @DominicKramer who is much more familiar with this codebase than myself.
|
@mctavish and I have been in communication about this. I will review it as soon as I can. |
There was a problem hiding this comment.
This looks great! Thanks for helping with this. I had a few nits, and there are some test failures to address, but it looks good. @bcoe How would you like to proceed with this? Should we have a separate tagged release as you mentioned doing before landing this in master?
| } | ||
|
|
||
| /** Interfaces pulled from source-map-support. */ | ||
| interface Position { |
There was a problem hiding this comment.
Is there a reason we can't use these directly from source-map-support? Are they not exported from it?
| lineNumber: number, | ||
| colNumber: number | ||
| ): MapInfoOutput | null { | ||
| const sms = this.safeRequireSourceMapSupport(); |
There was a problem hiding this comment.
Nit: It could be easier to read if you had
if (!sms) {
return null
}
...
| baseScriptPath, | ||
| line, | ||
| column | ||
| // Presently it is not possible to precisely disambiguate the script |
There was a problem hiding this comment.
The config has a workingDirectory configuration that could be useful.
| cb, | ||
| breakpoint, | ||
| StatusMessage.BREAKPOINT_SOURCE_LOCATION, | ||
| utils.messages.INVALID_BREAKPOINT // FIXME: Temporary just to check code flow. |
There was a problem hiding this comment.
Please address this before landing. Thanks.
|
gentle bump we have a major release coming up - any chance of us landing this along with it? |
|
@DominicKramer sorry for the slow response. I need to give this a more thorough review. If we do have a specific application in mind, I don't think it's a bad idea to release it to a @mctavish, could I bother you to flesh out the description of this pull request with a more thorough description of the feature, and the motivation 👌 |
product-auto-label
bot
added
the
api: clouddebugger
|
👋 is this still work we want to land eventually? |
|
I'll close this for now; the forked repository still exists so it can be revived later if the demand for the feature returns. |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕