module: expose Module._runInThisContext

[indutny]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

module

Description of change

Module._runInThisContext can be overridden to store/load cached
compilation artifacts when embedding node.js .

cc @nodejs/collaborators @zcbenz

[indutny]

CI: https://ci.nodejs.org/job/node-test-pull-request/2326/

[jasnell]

hmmm... not sure about this one. What are the ramifications of exposing this?

[indutny]

Actually none. It is already possible to override this through overriding vm.runInThisContext. However it makes things more complex for embedders.

The rationale behind this patch is that I have recently tried to add V8 code cache to electron and it turned out that it is notoriously hard to do it in a clean way without copy-pasting code from the core.

[ofrobots]

@indutny Can you elaborate on the use-case for exposing the code cache to a node embedder? What would electron do with the cached artifacts?

[indutny]

@ofrobots what's the point of introducing them in V8 in a first place? :) To speed up the compilation and to reduce the startup time?

[jasnell]

I guess what I'd prefer to see on this is a proper public API as opposed to
exposing _runInThisContext directly.
On Apr 19, 2016 4:33 PM, "Fedor Indutny" notifications@github.com wrote:

Actually none. It is already possible to override this through overriding
vm.runInThisContext. However it makes things more complex for embedders.

The rationale behind this patch is that I have recently tried to add V8
code cache to electron and it turned out that it is notoriously hard to do
it in a clean way without copy-pasting code from the core.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#6288 (comment)

[indutny]

@jasnell maybe... but we don't even have a documentation for Module.wrap and other existing methods of require('module'). Perhaps it is worth doing it prefixed first, and then moving to unprefixed version in the future?

[jasnell]

Or... Perhaps only expose this if the --embedded flag is set on compile?
(See #6266).
On Apr 19, 2016 5:58 PM, "Fedor Indutny" notifications@github.com wrote:

@jasnell https://github.com/jasnell maybe... but we don't even have a
documentation for Module.wrap and other existing methods of
require('module'). Perhaps it is worth doing it prefixed first, and then
moving to unprefixed version in the future?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6288 (comment)

[indutny]

@jasnell I'd rather not choose the second option. The embedder's code should work on regular node as much as possible. This is useful for testing, and code sharing as well.

[bnoordhuis]

-1 from me, feels too much like a hack.

[benjamingr]

I feel like this should be properly discussed first and that the API should look very different.

[steelbrain]

Bump

[jasnell]

@nodejs/ctc ... please weigh in on this.

[rvagg]

-0, every _ adds to our API debt, we know that users come to depend on them as much as "documented" APIs. I'm really not keen on exposing a hack because we don't have time to come up with something better.

Are you imagining embedders would replace this with their own wrapper to intercept for the purpose of capturing or inserting the cached version? That use is kind of like module.export.somethingPrivateThatIWantForMyTestSuite = ... which is a bit of an antipattern.

I'd even be happier with adding interceptor callbacks to the _extensions functions, something like 3rd and 4th arguments called loadCachedDataFor and saveCachedDataFor so you could override the .js extension function and add those two functions to insert and get cached data given a filename. But it seems like it might need an EP to discuss this further.

[ChALkeR]

-1. If we want to support this, it should be a public documented api, and it most likely shouldn't be prefixed with a _.

If we don't want to support that — what's the reason for exporting it in the current state? To achieve something that is not supported by the API, embedders could just add their patches on top and support those themselves.

[rvagg]

even exporting it in its current state (without _) would be a hack, it's exporting it purely for the purpose of monkeypatching, I think we can do better than that

[rvagg]

CTC moved this discussion back here - we'd like to find a more elegant way of exposing this API

[jasnell]

@nodejs/ctc ... looks like the was never any follow up on this. Recommend closing if we're not going to pursue it.

[fhinkel]

Feel free to reopen this PR if you get back to working on it.