Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Mocking use cases #549

Closed
guybedford opened this issue Aug 26, 2020 · 13 comments
Closed

Mocking use cases #549

guybedford opened this issue Aug 26, 2020 · 13 comments

Comments

@guybedford
Copy link
Contributor

As brought up by @boneskull today, we didn't have the time to discuss the full set of solutions to these problems.

Specifically, is a custom --loader the only way to mock, or are there other techniques that might make this easier and possible to do without spawning a new process.

@boneskull if you'd like to discuss this again for next week's agenda we can certainly do that - the process note is specifically that we do usually have a specific agenda to get through each week. Adding the agenda label here could bring it up again.

@guybedford
Copy link
Contributor Author

Furthermore, if --loader is the primary mechanism if we have a good example loader. The one mentioned is no longer supported since the deprecation of dynamicInstantiate.

@guybedford
Copy link
Contributor Author

I believe https://gist.github.com/guybedford/e6687e77d60739f27d1b5449a2872bf4 can be updated relatively simply to work without dynamicInstantiate. @giltayar also did some excellent investigations here in his post.

@MylesBorins MylesBorins added the modules-agenda To be discussed in a meeting label Aug 26, 2020
@giltayar
Copy link

I implemented a mocking library for testdouble, and I have to say that the experience was really nice. The loader API makes sense and works. And the knowledge that it's not a hack that may break in a future version (well, it will be once it's not experimental...) is reassuring.

You can see the details in my writeup: https://dev.to/giltayar/mock-all-you-want-supporting-es-modules-in-the-testdouble-js-mocking-library-3gh1

So: loaders are a great way to implement a mocking library.

Having said that, I believe that @boneskull is right in that the DX around loaders are less than optimal:

  1. Temporary: a bug around loaders and files with no extensions (e.g. mocha). But those are just bugs (Loader is not loaded when using "-e ..." node#33303) and not a real problem.
  2. The need to add --loader. Yes, it sounds like a quibble, but when I'm importing a mocking a package, it should just work, and that is the expectation of the user coming from CJS. Moreover, when using a test runner (e.g. Jest, Mocha, ...) I don't want to go and dig into the test runner documentation to understand how to make those test runners use the loader when they run the test (because many of them run the tests as subprocesses).
  3. No multiple loaders: if I'm already using a loader for, say, APM, or transpiling, then I am barred from using the mocking loader, which is a shame. But that is true for all loaders: I don't believe ESM loaders will be "production ready" till we define the semantics around multiple loaders and implement them.

@qballer
Copy link
Contributor

qballer commented Aug 31, 2020

The upcoming tooling meeting this Friday is going to have a discussion about this topic: nodejs/tooling#84. It seems reasonable that there would be an API in modules which exposes the cache.

@MylesBorins
Copy link
Contributor

@qballer I believe that one of the challenges to exposing the cache is doing so any remaining spec compliant. I'm also not 100% if V8 even exposes the cache to us.

/cc @bmeck who knows more

@giltayar
Copy link

@qballer as I've shown in my implementation of mocking for ESM, you don't really need access to the cache to implement mocking. See my writeup, which I've linked to above. The deltas for a good experience with mocking around not around the cache but around multiple loaders and some kind of api access to activating a loader.

Of course, other use cases for access to cache may be needed (hot module replacement would be a good one). But for mocking purposes, no need for that.

@jkrems
Copy link
Contributor

jkrems commented Aug 31, 2020

I'm also not 100% if V8 even exposes the cache to us.

I'm pretty sure it doesn't because there's no "cache" (in the CommonJS sense) for modules. It's variables that are shared across scopes, not arbitrary values assigned to module.exports. So updating the "cache" for export const x = 42 is asking for an API to change the binding of a const variable. Throw in the link phase and adding/removing exports, and it becomes super weird.

As @giltayar showed, a promising direction is to generate facade modules that have mutation APIs for changing their exported bindings. It's something we could add to node core but it would likely still require a startup-argument, making the UX roughly the same as having it as an ecosystem loader.

@boneskull
Copy link

boneskull commented Aug 31, 2020

(The tooling group meeting this Friday is not going to be discussing this issue, as we have something else scheduled that is not reflected by the current meeting issue)

@boneskull
Copy link

@giltayar has outlined the issues very well.

While I'm happy to provide more input in a modules meeting, I don't have much domain knowledge of ESM and loaders, and am oblivious to any impediments (or philosophical differences) that would hamstring an implementation of, say, programmatic loader usage. So maybe it's good to ask: are there any questions I should come prepared to answer?

@bmeck
Copy link
Member

bmeck commented Aug 31, 2020

@boneskull

One of the biggest questions about programmatic usage is how to avoid it from being used inappropriately, --loader is meant to have some level of isolation from application code and not meant to do things in the same way that caused various require APIs to become deprecated. This goes so far as to start leaking into policies to some extent as 3rd party loaders can invalidate guarantees of such policy configurations (which is intended!). This just needs a thorough write up.

In general I've have found the flag sufficient to prevent an arbitrary 3rd party module from taking over the loading process since it is done at bootstrap. The key of finding a way to prevent usage and/or limit it is key here. If the problem is having any configuration done we could look at a drop privileges style approach where you can drop the ability to inject loaders after an API is called, but that likely would still want a flag to enable the space of time were you can inject loaders. Similarly --require does have precedent of running prior to --frozen-intrinsics so it isn't a new concept.

Additionally, a lot of effort has gone in to make loaders at least in theory compatible with ahead of time tooling perhaps to the detriment of runtime tooling. Discussion of how to keep ahead of time tooling working in these contexts is pretty important in particular so that we can have a model for what is/is not guaranteed. Removing any guarantees about ahead of time tooling is likely untenable, but creating carve outs seems good.

I would note that in my opinion we don't need to necessarily have 100% compatibility with any given workflow for how mocking should be done as various issues do exist due to the design of ESM so it isn't a simple Object graph in JS unfortunately. Phrasing things around the specific needs or workflows and in particular avoiding mixing topics like mocking and unloading would be ideal. In general, unloading simply isn't workable, but altering the global (not local) module resolution for new (specifier, referrer) pairs is possible (this will not cause old modules to be unloaded and may cause version mismatches).

@GeoffreyBooth
Copy link
Member

@giltayar If I remember correctly, didn't the approach you found involve using query strings to load new modules in place of older specifiers? So package foo initially resolves to ./foo.js, but then on hot reload it resolves to ./foo.js?1 or somesuch? And the problem was that the earlier ./foo.js never gets unloaded, and then ./foo.js?1 never gets unloaded when ./foo.js?2 is created, etc.; so as time goes by during development, the Node process gradually uses more and more memory. Am I remembering accurately?

If so, then it's not like that approach fully solves the issue: users would want a way to do hot reload without gradually running out of memory. I think we were discussing V8's setScriptSource for that reason.

@bmeck
Copy link
Member

bmeck commented Aug 31, 2020

@GeoffreyBooth that is correct. However, if a Module is linked it won't be garbage collected even if you never reference it again in V8's implementation. Per setScriptSource the issues with supporting Module are not high priority for V8 and it would not re-run the top scope so it might not be a complete fix.

@MylesBorins
Copy link
Contributor

Removing the label, we've discussed this extensively and @giltayar has one way to solve this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants