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

Allow to load modules in non strict mode #448

Merged
merged 3 commits into from Aug 28, 2022
Merged

Conversation

XmiliaH
Copy link
Collaborator

@XmiliaH XmiliaH commented Jul 5, 2022

Add option to override if modules should be loaded in strict mode or not.

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`).
Copy link

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.

Copy link
Collaborator Author

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).

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... 😄

Copy link
Collaborator Author

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.

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.

Copy link
Collaborator Author

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 on timeout. If timeout 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 to sandbox by default.
  • Setting require.strict depending on strict setting.

@XmiliaH XmiliaH merged commit 58478a5 into patriksimek:master Aug 28, 2022
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