-
Notifications
You must be signed in to change notification settings - Fork 295
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
Allow to load modules in non strict mode #448
Conversation
README.md
Outdated
@@ -144,6 +144,8 @@ Unlike `VM`, `NodeVM` allows you to require modules in the same way that you wou | |||
* `require.context` - `host` (default) to require modules in the host and proxy them into the sandbox. `sandbox` to load, compile, and require modules in the sandbox. Except for `events`, built-in modules are always required in the host and proxied into the sandbox. | |||
* `require.import` - An array of modules to be loaded into NodeVM on start. | |||
* `require.resolve` - An additional lookup function in case a module wasn't found in one of the traditional node lookup paths. | |||
* `require.customRequire` - Use instead of the `require` function to load modules from the host. | |||
* `require.strict` - `false` to disable loading modules in strict mode (default: `true`). |
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.
How does the value of require.strict
impact the interpretation of code that includes (or doesn't include) a use strict;
directive?
I think it might help to clarify that here, as by the current wording I'd assume that use strict;
will be ignored when require.strict
is set to 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.
Would the following be better?
false
to not force strict mode on modules loaded by require
(default: true
).
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, and please forgive this one last grumble about the default value being true
here... 😄
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.
The default is true to have backwards compatibility.
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.
Worth the breaking change to have better compatibility with a vanilla node runtime, IMO. But I rest my case.
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.
At some point I would like to bump the minor version and change some default settings to be more secure and reasonable. This would include:
- Setting
allowAsync
depending ontimeout
. Iftimeout
is set disable async by default. - Remove
coffeescript
as compiler. - Make
require.resolve
required to load host modules. (Makes it easier for bundler as there won't be a dynamic require any more). - Change
require.context
tosandbox
by default. - Setting
require.strict
depending onstrict
setting.
Add option to override if modules should be loaded in strict mode or not.