loaders icon indicating copy to clipboard operation
loaders copied to clipboard

Proposal: Moving hooks on thread

Open GeoffreyBooth opened this issue 1 year ago • 5 comments
trafficstars

Following up https://github.com/nodejs/loaders/issues/200#issuecomment-2176788574, this is a draft proposal for option 3 from #201. This builds on #198 and assumes that that proposal would be implemented first.

GeoffreyBooth avatar Jun 22 '24 17:06 GeoffreyBooth

@nodejs/loaders plus the participants of https://github.com/nodejs/loaders/issues/103: @cspotcode @giltayar @arcanis @bumblehead @bizob2828 plus participants of https://github.com/nodejs/node/pull/53332: @VoltrexKeyva @Flarna @alan-agius4

GeoffreyBooth avatar Jun 22 '24 17:06 GeoffreyBooth

Some support for this.

My loader is an import interceptor. The main use case is full mocks in tests. On the principle "don't hide errors", I compare the given mock to the shape of original module. The attempt fails if the mock tries to add a name that doesn't exist, omit a name that does exist, or set a value for a name that's ambiguous. If the mock does any of these things then the test passes while production fails.

// util.mjs
export function frizzle () {}
// dazzle.mjs
import { frizzle, frazzle } from './util.mjs'
export class Dazzle { /* use frizzle and frazzle */ }
// test.mjs
const frizzle = sinon.stub()
const frazzle = sinon.stub() // not present in the authentic module
pose('./util.mjs', { frizzle, frazzle })
const { Dazzle } = await import('./dazzle.mjs')
// PhantomExport: Attempted to add an absent name to the module

Achieving this requires analyzing the full module graph. A naive approach would simply load each module but I'd prefer to avoid that. Where the entire module is mocked no exports can ever be called so instantiating is a waste, it may take significant time if it fronts a large graph of eg 100 modules, and there may load time modification of state that I'd like to isolate the test from.

I saw an APM rep in one of the meetings say they really need to load the authentic module so they can wrap it with tracing functions. Just want to register here that I kind of have the opposite use case. I'd specifically like to analyze all the source without evaluating it.

This hook enables exactly these things, so I'm in strong support.

ghost avatar Jul 14 '24 23:07 ghost

I am not sure if I understand the use case correctly but sounds like it can be addressed with the link hook proposed in https://github.com/nodejs/loaders/pull/198, which is invoked after module compilation and before evaluation, at that point you have the real export names parsed by V8, but the modules are not evaluated yet so you can perform the tests there and if it throws, the graph would not be evaluated at all. That also saves you the cost of parsing the modules to get the export names by yourself, since you can piggypack on the names readily parsed by V8.

joyeecheung avatar Aug 05 '24 17:08 joyeecheung

It would be awesome, that's definitely the most complicated piece. It's really close. I'd need the list of ambiguous names to fully replace it. Would really prefer to let the engine handle this.

ghost avatar Aug 05 '24 18:08 ghost

It's seeming better and better the more I mull. I misunderstood link at first as posteval precache, giving access to the real exports but allowing last minute override. But actually it's just the shape and allows preeval override. It's perfect.

It will work without the ambiguous names, that's really just a special case of undefined.

ghost avatar Aug 05 '24 18:08 ghost