-
-
Notifications
You must be signed in to change notification settings - Fork 689
feat: add runtime feature "detection" #4545
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
Conversation
| function detectRuntimeFeatureByNodeModule (moduleName) { | ||
| try { | ||
| require(moduleName) | ||
| return true | ||
| } catch (err) { | ||
| if (err.code !== 'ERR_UNKNOWN_BUILTIN_MODULE') { | ||
| throw err | ||
| } | ||
| return false | ||
| } | ||
| } |
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.
Could it be better to return the module itself as well?
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.
I would actually try to avoid to have a second require cache or something like that.
I actually think also about a check for zsdt or maybe markAsClonable.
It could help have us to test branches which are supported by the engine.
|
@metcoder95 |
mcollina
left a comment
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
gurgunday
left a comment
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
Basically centralizes the detection of crypto and sqlite. Also I rewrote some code to avoid the loading of crypto and sqlite module to early.
Maybe it makes sense to have some smaller PRs, if you dont think runtimeFeatures instance/class is a good solution. But with runtimeFeatures Instance, we could actually disable specific features and test it.
We could actually also have a little crypto ponyfill
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status