feat: throw if a boundary file is not dynamically imported

.changeset/five-rats-clean.md

@@ -0,0 +1,16 @@
+---
+"hot-hook": minor
+---
+
+Now Hot-Hook will throw an error when a file marked as "boundary" is not dynamically imported.
+
+In AdonisJS, we had a few users complaining about having to restart the server to see the changes applied. Generally, the cause of this was a controller file not dynamically imported:
+
+```ts
+import PostsController from './app/controllers/posts_controller.js'
+router.get('/posts', [PostsController, 'index'])
+```
+
+Before this new version, this code did not throw an error, but it did not work either. You had to reload the server to see the changes. Now Hot-Hook will throw an error for this kind of case.
+
+I invite you to reread the readme if you want to understand why a dynamic import is necessary for Hot-Hook to work correctly.

packages/hot_hook/package.json

@@ -35,6 +35,7 @@
   "dependencies": {
     "chokidar": "^3.6.0",
     "fast-glob": "^3.3.2",
+    "parse-imports": "^2.2.1",
     "picomatch": "^4.0.2",
     "read-package-up": "^11.0.0"
   },

packages/hot_hook/src/dynamic_import_checker.ts

@@ -0,0 +1,48 @@
+import { readFile } from 'node:fs/promises'
+import { parseImports } from 'parse-imports'
+import { relative } from 'node:path'
+
+/**
+ * This class is responsible for checking if a given specifier
+ * is imported dynamically from a given parent file.
+ * Otherwise we will throw an error since we cannot make the file reloadable
+ *
+ * We are caching the results to avoid reading the same file multiple times
+ */
+export class DynamicImportChecker {
+  private cache: Map<string, Map<string, boolean>> = new Map()
+  private projectRoot: string
+
+  constructor(projectRoot: string) {
+    this.projectRoot = projectRoot
+  }
+
+  async ensureFileIsImportedDynamicallyFromParent(parentPath: string, specifier: string) {
+    const cacheKey = parentPath
+    if (this.cache.has(cacheKey) && this.cache.get(cacheKey)!.has(specifier)) {
+      return this.cache.get(cacheKey)!.get(specifier)
+    }
+
+    const parentCode = await readFile(parentPath, 'utf-8')
+    const imports = [...(await parseImports(parentCode))]
+
+    const isFileDynamicallyImportedFromParent = imports.some((importStatement) => {
+      return importStatement.isDynamicImport && importStatement.moduleSpecifier.value === specifier
+    })
+
+    const currentCache = this.cache.get(cacheKey) ?? new Map()
+    this.cache.set(cacheKey, currentCache.set(specifier, isFileDynamicallyImportedFromParent))
+
+    if (!isFileDynamicallyImportedFromParent) {
+      throw new Error(
+        `The import "${specifier}" is not imported dynamically from ${relative(this.projectRoot, parentPath)}.\nYou must use dynamic import to make it reloadable (HMR) with hot-hook.`
+      )
+    }
+
+    return isFileDynamicallyImportedFromParent
+  }
+
+  invalidateCache(key: string) {
+    this.cache.delete(key)
+  }
+}

packages/hot_hook/src/loader.ts

@@ -1,14 +1,15 @@
 import chokidar from 'chokidar'
+import { fileURLToPath } from 'node:url'
 import { realpath } from 'node:fs/promises'
 import { MessagePort } from 'node:worker_threads'
-import { fileURLToPath } from 'node:url'
 import { resolve as pathResolve, dirname } from 'node:path'
 import type { InitializeHook, LoadHook, ResolveHook } from 'node:module'
 
 import debug from './debug.js'
 import { Matcher } from './matcher.js'
 import DependencyTree from './dependency_tree.js'
 import { InitializeHookOptions } from './types.js'
+import { DynamicImportChecker } from './dynamic_import_checker.js'
 
 export class HotHookLoader {
   #options: InitializeHookOptions
@@ -19,6 +20,7 @@ export class HotHookLoader {
   #pathIgnoredMatcher!: Matcher
   #dependencyTree: DependencyTree
   #hardcodedBoundaryMatcher!: Matcher
+  #dynamicImportChecker!: DynamicImportChecker
 
   constructor(options: InitializeHookOptions) {
     this.#options = options
@@ -28,6 +30,7 @@ export class HotHookLoader {
     if (options.root) this.#initialize(options.root)
 
     this.#dependencyTree = new DependencyTree({ root: options.root })
+    this.#dynamicImportChecker = new DynamicImportChecker(this.#projectRoot)
     this.#messagePort?.on('message', (message) => this.#onMessage(message))
   }
 
@@ -59,6 +62,12 @@ export class HotHookLoader {
     const filePath = pathResolve(relativeFilePath)
     const realFilePath = await realpath(filePath)
 
+    /**
+     * Invalidate the dynamic import cache for the file since we
+     * gonna need to recheck the dynamic imports.
+     */
+    this.#dynamicImportChecker.invalidateCache(filePath)
+
     /**
      * If the file is an hardcoded reload file, we trigger a full reload.
      */
@@ -185,6 +194,10 @@ export class HotHookLoader {
       const isHardcodedBoundary = this.#hardcodedBoundaryMatcher.match(resultPath)
       const reloadable = context.importAttributes?.hot === 'true' ? true : isHardcodedBoundary
 
+      if (reloadable) {
+        this.#dynamicImportChecker.ensureFileIsImportedDynamicallyFromParent(parentPath, specifier)
+      }
+
       this.#dependencyTree.addDependency(parentPath, { path: resultPath, reloadable })
     }
 

packages/hot_hook/tests/dynamic_import_checker.spec.ts

@@ -0,0 +1,33 @@
+import { test } from '@japa/runner'
+import { DynamicImportChecker } from '../src/dynamic_import_checker.js'
+import { join } from 'node:path'
+
+test.group('Dynamic Import Checker', () => {
+  test('Throw if given specifier is not dynamically importedf', async ({ assert, fs }) => {
+    await fs.create(
+      'app.ts',
+      `
+      import './foo'
+      await import('./bla')
+
+      import '#app/aliases'
+      await import('#app/aliases-bla')
+      `
+    )
+
+    const checker = new DynamicImportChecker(fs.basePath)
+
+    const path = join(fs.basePath, 'app.ts')
+
+    await assert.rejects(async () => {
+      await checker.ensureFileIsImportedDynamicallyFromParent(path, './foo')
+    })
+
+    await assert.rejects(async () => {
+      await checker.ensureFileIsImportedDynamicallyFromParent(path, '#app/aliases')
+    })
+
+    assert.isTrue(await checker.ensureFileIsImportedDynamicallyFromParent(path, './bla'))
+    assert.isTrue(await checker.ensureFileIsImportedDynamicallyFromParent(path, '#app/aliases-bla'))
+  })
+})