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

Module.builtinModules should not include "inspector" when in a worker #22676

Closed
jdalton opened this issue Sep 3, 2018 · 6 comments
Closed

Module.builtinModules should not include "inspector" when in a worker #22676

jdalton opened this issue Sep 3, 2018 · 6 comments
Labels
inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support.

Comments

@jdalton
Copy link
Member

jdalton commented Sep 3, 2018

The recommended way to sniff for "inspector" support without using process.binding() has been to use Module.builtinModules. However, when in a worker the Module.builtinModules array still contains "inspector" resulting in an error being thrown when the module is required.

node/lib/inspector.js

Lines 17 to 18 in e570ae7

if (!Connection || !require('internal/worker').isMainThread)
throw new ERR_INSPECTOR_NOT_AVAILABLE();

@addaleax addaleax added inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support. labels Sep 3, 2018
@addaleax
Copy link
Member

addaleax commented Sep 3, 2018

I’m not sure there’s much to do here, given that Workers are still experimental and #21364 is pretty close to landing.

@jdalton
Copy link
Member Author

jdalton commented Sep 3, 2018

Feel free to close when it lands. It's something to keep in mind for future additions though. Having to require the module in a try-catch isn't great (unintended code execution, gross-try-catch, etc.).

Update:

Do we know if the other case, !Connection, is covered by Module.builtinModules too?

@mscdex
Copy link
Contributor

mscdex commented Sep 3, 2018

Also, is there a reason why there are 'v8/*' and 'node-inspect/*' entries in require('module').builtinModules? Is this list not meant for public consumption? I would have expected only node core modules to be in there...

@devsnek
Copy link
Member

devsnek commented Sep 3, 2018

@mscdex anything that can be require()ed is in the list. the v8 and node-inspect stuff was going to be separately deprecated.

@jdalton
Copy link
Member Author

jdalton commented Sep 12, 2018

Related to #22769.

@Trott
Copy link
Member

Trott commented Nov 21, 2018

I'm closing because #21364 landed. If that isn't right and this should remain open, by all means, correct my error and re-open this!

@Trott Trott closed this as completed Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

5 participants