braidnetworks / dynohot

Hot module reloading for nodejs
ISC License
89 stars 3 forks source link

Programmatic registration is not working #5

Closed kireerik closed 8 months ago

kireerik commented 10 months ago

index.js

import.meta.hot.accept()

  main.js

import {register} from 'module'

register('dynohot', import.meta.url)

or

import('dynohot/register')

 

import('./index')

 

node main
        import.meta.hot.accept()
                        ^

TypeError: Cannot read properties of undefined (reading 'accept')

  command line registration works as expected:

node --import dynohot/register .

or

node --loader dynohot .
laverdet commented 10 months ago

We unfortunately can't enable dynamic registration because then we don't have a "top" module which serves as the basis for module graph traversal. I've been pretty vocal in nodejs loader discussions that register was a bad design choice and should be revisited.

kireerik commented 10 months ago

Yes, I noticed it before: https://github.com/braidnetworks/dynohot/blob/3f1aa250c8e5018a54180f425fa6c603d9d01779/loader/register.ts#L3-L6

Yes, it feels a bit weird using it.

Most related comment:

Currently I only have a bad workaround for this which is to register it both ways but that of course leads to a minor issue and memory leaks.

kireerik commented 10 months ago

What do you roughly mean by that in that case we don't have a "top" module?

laverdet commented 10 months ago

The hot update algorithm starts here: https://github.com/braidnetworks/dynohot/blob/3f1aa250c8e5018a54180f425fa6c603d9d01779/runtime/controller.ts#L444

We just start from "top" or "main" and traverse the entire dependency graph to dispatch a hot update. In the case of dynamic register then there is no "main". If there is no main then we would need to start the traversal from the module which was updated and work our way down. In theory it would be faster because you could run into an accept before reaching a root module. But in practice it's a lot easier to keep track of dependencies than it is to keep track of dependents.

kireerik commented 9 months ago

To my understanding registered custom loaders should apply to subsequent dynamic imports in the module where the custom loaders are registered (https://nodejs.org/api/module.html#enabling).

laverdet commented 9 months ago

It is not a problem with the loader chain, it is the dynohot runtime. As I mentioned before the "loader" side of things is actually very simple. It's a stateless code transformation.

If we allow dynamic registration of the dynohot loader runtime then the application no longer forms a neat graph. The commit message of 0586bc40a658e5a2625dc4b4e5f57776311ce06c has some more information there. It becomes, I believe, impossible to detect module update deadlocks in this case. I think we can probably make it work by making an invisible "top" module and starting the graph from there but I don't know if the complexity is worth it for this curiosity. I'm of the opinion that loaders should be declared statically and not dynamically due to the way they affect the module graph.

kireerik commented 9 months ago

Ah, I see. I think I understand some of it now.

So could we separate the dynohot loader and the dynohot runtime? So then we could register the dynohot loader (programmatically as well) and start the runtime (also programmatically (and manually (not as part of the loader))) separately.

kireerik commented 8 months ago

Some time ago I consulted with bloop (asked it couple of questions) on dynohot and I started to understand the problem a bit more. So I can see that it is hard to solve it nicely.

I think full dynamic registration support would mean that the main module would not be hot reloadable (which is okay in my use case).

Now as a better workaround I am using a separately imported module to register the loaders

laverdet commented 8 months ago

Moving all your loaders to a separate --import is a good solution, I hadn't thought of that.