Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Programmatic registration is not working #5

Closed
kireerik opened this issue Jan 21, 2024 · 9 comments
Closed

Programmatic registration is not working #5

kireerik opened this issue Jan 21, 2024 · 9 comments

Comments

@kireerik
Copy link

kireerik commented Jan 21, 2024

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
Copy link
Contributor

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
Copy link
Author

kireerik commented Jan 21, 2024

Yes, I noticed it before:

// I can not believe they did this.
register("dynohot", {
parentURL: import.meta.url,
});

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
Copy link
Author

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

@laverdet
Copy link
Contributor

The hot update algorithm starts here:

const initialResult = traverseDepthFirst(

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
Copy link
Author

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
Copy link
Contributor

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 0586bc4 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
Copy link
Author

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
Copy link
Author

kireerik commented Mar 9, 2024

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
Copy link
Contributor

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

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

No branches or pull requests

2 participants