Skip to content
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 support for proper handling of file protocol (in windows) #201

Merged
merged 2 commits into from Jan 16, 2018

Conversation

kriszyp
Copy link
Contributor

@kriszyp kriszyp commented Jan 16, 2018

Trying to debug node with DevTools with transpiled code on Windows is a mess. First, this is not the fault of node-source-map-support at all. It works great on windows on its own. But the hacks necessary to get this to work do break node-source-map-support.

The basic problem is this: Node uses OS file paths, and DevTools doesn't understand OS file paths, it only understands URIs (including file:// URIs). So to actually get DevTools to comprehend source maps (so that you can set breakpoints and step through source code), you need to map your node modules to file:// URIs, so that DevTools can then do the relative path lookups to find your source maps/code. This means your modules that registered in Node need to have filenames like "file:///C:/dev/my-module.js". And this is where the most critical issue in node-source-map-support comes in: supportRelativeURL will break that up into "file://" and "/C:/dev/my-module.js" which path.resolve incorrectly computes to something like "C:/C:/dev/module.js" (plus relative path). And because supportRelativeURL can't be replaced by options, I think this actually needs to be fixed in node-source-map-support.

I figured while I was fixing this, I would also fix the default retrieveFileHandler to be able to properly handle file:// support (since existsSync/readFileSync doesn't on its own). Although technically you can provide a custom file handler to override this, as a user.

If you are wondering how I map node modules to file:// URLs, in our application I have been monkey patching the vm.runInThisContext function:

  vm.runInThisContext = function(wrapper, options) {
    if (options && options.filename && options.filename.match(/\.js$/) && options.filename.indexOf('node_modules') == -1) {
      options.filename = 'file:///' + options.filename.replace(/\\/g, '/')
    }
    return runInThisContext.apply(this, arguments)
  }

I suppose bonus support would be to actually add an option that does this for the user.

With this all in place, I now have the complete zen harmony of perfectly source-mapped modules in my stack traces and dev tools. I can set breakpoints in source code, I can click on stack traces, and go to the right source in dev tools. It all works beautifully.

I haven't done anything with tests. I wasn't sure how much of this would be willing to be accepted. Let me know if you want anything else for this PR.

@LinusU
Copy link
Collaborator

LinusU commented Jan 16, 2018

Seems like a good approach 👍

Any idea why the tests are failing on 0.x?

@LinusU
Copy link
Collaborator

LinusU commented Jan 16, 2018

I don't see how this could break anything that is working right now, so lets ship it! :shipit:

Thank you for the PR

@LinusU LinusU merged commit 4229886 into evanw:master Jan 16, 2018
@LinusU
Copy link
Collaborator

LinusU commented Jan 16, 2018

Published as 0.5.1

@kriszyp
Copy link
Contributor Author

kriszyp commented Jan 16, 2018

Wow, that was fast, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants