feat: throw if HMR is enabled and controller is directly imported

[RomainLanz]

Hey there! 👋🏻

This PR will cause the router to throw the exception E_DIRECT_CONTROLLER_IMPORT if users directly import the controller and HMR is enabled.

Why is this change needed?

If HMR is enabled (and it is by default), we can only hot-replace code that has been layz imported. Since all controllers are marked as "boundary" by default, your code will simply break when not lazy-import them.

import HomeController from '#controllers/home_controller'

router.get('/', [HomeController, 'render'])

For example, with the code above, anytime I modify the HomeController code, I will see a message in my console saying the file was invalidated, but this is not the case, and my modification will never be taken into account unless I restart the process.

This is an issue that has been reported already multiple times since we activated HMR by default.

[thetutlage]

I fundamentally don't agree with this approach on various fronts.

• The routers shouldn't be concerned how a controller is imported.
• I have use-cases/apps where I am registering routes from providers inside a package and these controllers are pre-imported.

Now coming to the actual question, which is "People experience issues when they have HMR enabled and controller is not lazy imported"

So far my understanding was, this use-case will perform a full-reload. But @Julien-R44 mentioned in Discord, that's not the case. So I am trying to first understand why that's not a case.

When we form the dependencies graph, can't we detect the direct imports and have them removed from the boundaries files?

[RomainLanz]

I have use-cases/apps where I am registering routes from providers inside a package and these controllers are pre-imported.

Yes, that would probably not work in that case, because your provider will throw when adding them.

So far my understanding was, this use-case will perform a full-reload.

Nope, it does work like that because it is a glob pattern that defines the boundary, and all controllers are in by default.

When we form the dependencies graph, can't we detect the direct imports and have them removed from the boundaries files?

Only @Julien-R44 can answer this question, I haven't looked into the code base.

[Julien-R44]

When we form the dependencies graph, can't we detect the direct imports and have them removed from the boundaries files?

Nope we can't. Via load and resolve we have no way of detecting whether a given import is lazy or not, unfortunately.

So from the moment a file is marked as boundary we consider this file is hot-reloadable and therefore lazily imported.

as far as I know, we don't have any solution at this level

[thetutlage]

Yeah, I will suggest not rushing with it, because some folks are struggling with something that can be fixed with a linter.

Meanwhile let's see if there is some other place (maybe within the hook) we can detect and throw an error.

[RomainLanz]

In the meanwhile, what do you think if we add a small "warning box" when you run your code that says:

Ensure to lazy-load your boundary since you are using HMR mode

We all know that people don't read the doc, and this issue can easily happen.

[thetutlage]

I think a logger warning is fine within the router for now.

Maybe something like this

"xxx controller must be lazy loaded in order to benefit from HMR"

[RomainLanz]

For anyone reading, we are currently waiting to see if Node.js can tell us if an import is dynamic or not. Related: https://github.com/nodejs/loaders/issues/204

[RomainLanz]

After some thoughts, I believe it would be better if we simply do not activate HMR by default.

HMR is awesome, but currently, it may creates some weird cases if you don't read about HMR first. So it could be an opt-in solution if you know about that stuff, until Node.js add something for us to make it better.

Also, we could then rename the Legacy watch mode to Cold Reload.

[thetutlage]

@Julien-R44 Do you think, there is some way we can make it work? Otherwise, as @RomainLanz suggested, we will have to revert back to complete reload and that will now slow things down because the Vite server will restart every time as well. Not an ideal solution as per me 😞

[RomainLanz]

It is no longer needed with https://github.com/Julien-R44/hot-hook/releases/tag/hot-hook%400.3.0.

Closing.