feat: add support for compileFunction allowing us to avoid the module wrapper
#9252
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SimenB
commented
Dec 1, 2019
SimenB
commented
Dec 1, 2019
SimenB
commented
Dec 1, 2019
SimenB
commented
Dec 1, 2019
| return compileFunction(code, params, { | ||
| filename, | ||
| parsingContext: this.context, | ||
| }) as any; |
thymikee
reviewed
Dec 1, 2019
thymikee
reviewed
Dec 1, 2019
SimenB
force-pushed
the
module-wrap
branch
2 times, most recently
from
4022404 to
8b200be
Compare
Member
Author
|
@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? |
SimenB
commented
Dec 1, 2019
| fakeTimersLolex: LolexFakeTimers | null; | ||
| moduleMocker: jestMock.ModuleMocker | null; | ||
| runScript<T = unknown>(script: Script): T | null; | ||
| compileFunction?<T = unknown>( |
Member
Author
There was a problem hiding this comment.
I've made it optional so we're not blocked by jsdom having support. It'll silently use it if it's ever implemented
SimenB
commented
Dec 1, 2019
| if ( | ||
| e instanceof SyntaxError && | ||
| // `instanceof` might come from the wrong context | ||
| e.name === 'SyntaxError' && |
Member
Author
There was a problem hiding this comment.
the SyntaxError thrown by compileFunction comes from the passed in context
SimenB
changed the title
compileFunction allowing us to avoid the module wrapper
SimenB
commented
Dec 1, 2019
Contributor
|
@SimenB works for me |
thymikee
approved these changes
Dec 2, 2019
This was referenced Jan 28, 2020
This was referenced Feb 4, 2020
This was referenced Feb 18, 2020
This was referenced Mar 3, 2020
This was referenced Mar 10, 2020
This was referenced Mar 24, 2020
This was referenced Apr 16, 2020
This was referenced Apr 30, 2020
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.compileFunctionwas addednode@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
moduleWrapperLengththing 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-wrapperseems 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