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

feat: add support for compileFunction allowing us to avoid the module wrapper #9252

Merged
merged 5 commits into from
Dec 2, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 1, 2019

Summary

This is essentially nodejs/node#21573. Doing this should make it easier for sourcemaps to do their thing since we don't inject arbitrary code around the code.

I made this into an option as vm.compileFunction was added node@10.10. Happy to not have an option and rather use feature detection? Also note that JSDOM does not support it, but I'll open up a PR shortly.

EDIT: jsdom/jsdom#2731

Mildly related to #8596 as it won't need the moduleWrapperLength thing anymore.

Docs: https://nodejs.org/api/vm.html#vm_vm_compilefunction_code_params_options

Test plan

I haven't added new tests yet, but existing tests should pass, na drunning locally with --no-module-wrapper seems to work correctly. If CI is happy, I'll flip the default and see what it says - hopefully the only failing tests at that point will be the ones for config

CHANGELOG.md Outdated Show resolved Hide resolved
return compileFunction(code, params, {
filename,
parsingContext: this.context,
}) as any;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it returns Function

@SimenB SimenB force-pushed the module-wrap branch 2 times, most recently from 4022404 to 8b200be Compare December 1, 2019 22:20
@SimenB
Copy link
Member Author

SimenB commented Dec 1, 2019

@thymikee I've reworked this to use feature detection instead of an option, and just dropped it from jsdom for now. It should be invisible to consumers. Thoughts?

@@ -44,6 +44,11 @@ export declare class JestEnvironment {
fakeTimersLolex: LolexFakeTimers | null;
moduleMocker: jestMock.ModuleMocker | null;
runScript<T = unknown>(script: Script): T | null;
compileFunction?<T = unknown>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it optional so we're not blocked by jsdom having support. It'll silently use it if it's ever implemented

@@ -17,7 +17,8 @@ export default function handlePotentialSyntaxError(
}

if (
e instanceof SyntaxError &&
// `instanceof` might come from the wrong context
e.name === 'SyntaxError' &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the SyntaxError thrown by compileFunction comes from the passed in context

@SimenB SimenB changed the title feat: add support for avoiding the module wrapper feat: add support for compileFunction allowing us to avoid the module wrapper Dec 1, 2019
@thymikee
Copy link
Collaborator

thymikee commented Dec 1, 2019

@SimenB works for me

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants