feat: full reload when a boundary file was wrongly imported (#11)

.changeset/blue-taxis-poke.md

@@ -0,0 +1,17 @@
+---
+'hot-hook': minor
+---
+
+Earlier, with this release https://github.com/Julien-R44/hot-hook/releases/tag/hot-hook%400.3.0 we were just throwing and thus killing the app when a boundary file was not dynamically imported because it prevented hot-hook from hot-reloading.
+
+Now, we no longer throw the error by default, we simply emit a message of type "hot-hook:full-reload" to the parent process, which will be responsible for restarting the entire app. This "hot-hook:full-reload" message is not new, it was already used for files that should trigger a full reload.
+
+If you still want to throw the error, then you can pass the `throwWhenBoundariesAreNotDynamicallyImported` option to true, when you call `hot.init` or in your `package.json`:
+
+```json
+{
+  "hotHook": {
+    "throwWhenBoundariesAreNotDynamicallyImported": true
+  }
+}
+```

.changeset/spotty-donkeys-doubt.md

@@ -0,0 +1,5 @@
+---
+'@hot-hook/runner': minor
+---
+
+The runner will now full-reload the application when a file changes and the boundaries are not dynamically imported.

examples/adonisjs/package.json

@@ -7,8 +7,8 @@
   "scripts": {
     "start": "node bin/server.js",
     "dev": "node ace serve --hmr",
-    "format": "prettier --write .",
-    "typecheck": "tsc --noEmit"
+    "typecheck": "tsc --noEmit",
+    "format": "prettier --write ."
   },
   "imports": {
     "#controllers/*": "./app/controllers/*.js",
@@ -31,8 +31,8 @@
   "devDependencies": {
     "@adonisjs/assembler": "^7.5.1",
     "@adonisjs/eslint-config": "^1.3.0",
-    "@adonisjs/prettier-config": "^1.3.0",
-    "@adonisjs/tsconfig": "^1.3.0",
+    "@adonisjs/prettier-config": "^1.4.0",
+    "@adonisjs/tsconfig": "^1.4.0",
     "@japa/assert": "^3.0.0",
     "@japa/plugin-adonisjs": "^3.0.1",
     "@japa/runner": "^3.1.4",

packages/hot_hook/src/dependency_tree.ts

@@ -33,6 +33,11 @@ interface FileNode {
    * Version of the file. Incremented when the file is invalidated
    */
   version: number
+
+  /**
+   * Whether the file is not dynamically imported where it should be
+   */
+  isWronglyImported?: boolean
 }
 
 export default class DependencyTree {
@@ -69,7 +74,10 @@ export default class DependencyTree {
   /**
    * Add a dependency to a file
    */
-  addDependency(parentPath: string, dependency: { path: string; reloadable?: boolean }): void {
+  addDependency(
+    parentPath: string,
+    dependency: { path: string; reloadable?: boolean; isWronglyImported?: boolean }
+  ): void {
     let parentNode = this.#pathMap.get(parentPath)
     if (!parentNode) return
 
@@ -82,10 +90,12 @@ export default class DependencyTree {
         dependents: new Set(),
         dependencies: new Set(),
         reloadable: dependency.reloadable || false,
+        isWronglyImported: dependency.isWronglyImported || false,
       }
       this.#pathMap.set(dependency.path, childNode)
     } else {
       childNode.reloadable = dependency.reloadable || false
+      childNode.isWronglyImported = dependency.isWronglyImported || false
     }
 
     childNode.parents?.add(parentNode)
@@ -117,7 +127,7 @@ export default class DependencyTree {
       if (!invalidatedFiles.has(currentPath)) {
         const node = this.#pathMap.get(currentPath)
         if (!node) continue
-        if (!this.isReloadable(currentPath)) continue
+        if (!this.isReloadable(currentPath).reloadable) continue
 
         node.version++
         invalidatedFiles.add(currentPath)
@@ -159,30 +169,38 @@ export default class DependencyTree {
    *  need to do a FULL RELOAD.
    * - If all paths to reach the ROOT file go through reloadable files, then it means we can do HMR !
    */
-  isReloadable(path: string): boolean {
+  isReloadable(path: string) {
     const node = this.#pathMap.get(path)
     if (!node) throw new Error(`Node ${path} does not exist`)
 
-    const checkPathToRoot = (currentNode: FileNode, visited: Set<string> = new Set()): boolean => {
+    const checkPathToRoot = (
+      currentNode: FileNode,
+      visited: Set<string> = new Set()
+    ): { reloadable: boolean; shouldBeReloadable: boolean } => {
+      if (currentNode.isWronglyImported) {
+        return { reloadable: false, shouldBeReloadable: true }
+      }
+
       if (currentNode.reloadable) {
-        return true
+        return { reloadable: true, shouldBeReloadable: true }
       }
 
       if (visited.has(currentNode.path)) {
-        return true
+        return { reloadable: true, shouldBeReloadable: true }
       }
 
       visited.add(currentNode.path)
 
       if (!currentNode.parents || currentNode.parents.size === 0) {
-        return false
+        return { reloadable: false, shouldBeReloadable: false }
       }
 
       for (const parent of currentNode.parents) {
-        if (!checkPathToRoot(parent, new Set(visited))) return false
+        const { reloadable, shouldBeReloadable } = checkPathToRoot(parent, new Set(visited))
+        if (!reloadable) return { reloadable: false, shouldBeReloadable }
       }
 
-      return true
+      return { reloadable: true, shouldBeReloadable: true }
     }
 
     const result = checkPathToRoot(node)
@@ -194,12 +212,12 @@ export default class DependencyTree {
     const isNodeModule = (path: string) => path.includes('node_modules')
 
     return Array.from(this.#pathMap.values()).map((node) => ({
-      path: relative(rootDirname, node.path),
-      boundary: node.reloadable,
-      reloadable: isNodeModule(node.path) ? false : this.isReloadable(node.path),
       version: node.version,
-      dependencies: Array.from(node.dependencies).map((n) => relative(rootDirname, n.path)),
+      boundary: node.reloadable,
+      path: relative(rootDirname, node.path),
       dependents: Array.from(node.dependents).map((n) => relative(rootDirname, n.path)),
+      dependencies: Array.from(node.dependencies).map((n) => relative(rootDirname, n.path)),
+      reloadable: isNodeModule(node.path) ? false : this.isReloadable(node.path).reloadable,
     }))
   }
 }

packages/hot_hook/src/dynamic_import_checker.ts

@@ -1,6 +1,5 @@
 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
@@ -11,16 +10,11 @@ import { relative } from 'node:path'
  */
 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)
+      return this.cache.get(cacheKey)!.get(specifier)!
     }
 
     const parentCode = await readFile(parentPath, 'utf-8')
@@ -33,12 +27,6 @@ export class DynamicImportChecker {
     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
   }
 

packages/hot_hook/src/errors/file_not_imported_dynamically_exception.ts

@@ -0,0 +1,9 @@
+import { relative } from 'node:path'
+
+export class FileNotImportedDynamicallyException extends Error {
+  constructor(parentPath: string, specifier: string, projectRoot: string) {
+    super(
+      `The import "${specifier}" is not imported dynamically from ${relative(projectRoot, parentPath)}.\nYou must use dynamic import to make it reloadable (HMR) with hot-hook.`
+    )
+  }
+}

packages/hot_hook/src/hot.ts

@@ -19,7 +19,12 @@ class Hot {
    */
   #onMessage(message: MessageChannelMessage) {
     if (message.type === 'hot-hook:full-reload') {
-      process.send?.({ type: 'hot-hook:full-reload', path: message.path })
+      process.send?.({
+        type: 'hot-hook:full-reload',
+        path: message.path,
+        shouldBeReloadable: message.shouldBeReloadable,
+      })
+
       this.#options.onFullReloadAsked?.()
     }
 
@@ -79,6 +84,8 @@ class Hot {
         boundaries: this.#options.boundaries,
         messagePort: this.#messageChannel.port2,
         rootDirectory: this.#options.rootDirectory,
+        throwWhenBoundariesAreNotDynamicallyImported:
+          this.#options.throwWhenBoundariesAreNotDynamicallyImported,
       } satisfies InitializeHookOptions,
     })
 

packages/hot_hook/src/loader.ts

@@ -10,6 +10,7 @@ import { Matcher } from './matcher.js'
 import DependencyTree from './dependency_tree.js'
 import { InitializeHookOptions } from './types.js'
 import { DynamicImportChecker } from './dynamic_import_checker.js'
+import { FileNotImportedDynamicallyException } from './errors/file_not_imported_dynamically_exception.js'
 
 export class HotHookLoader {
   #options: InitializeHookOptions
@@ -30,7 +31,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.#dynamicImportChecker = new DynamicImportChecker()
     this.#messagePort?.on('message', (message) => this.#onMessage(message))
   }
 
@@ -104,10 +105,14 @@ export class HotHookLoader {
      * If the file is not reloadable according to the dependency tree,
      * we trigger a full reload.
      */
-    const isReloadable = this.#dependencyTree.isReloadable(realFilePath)
-    if (!isReloadable) {
+    const { reloadable, shouldBeReloadable } = this.#dependencyTree.isReloadable(realFilePath)
+    if (!reloadable) {
       debug('Full reload (not-reloadable file) %s', realFilePath)
-      return this.#messagePort?.postMessage({ type: 'hot-hook:full-reload', path: realFilePath })
+      return this.#messagePort?.postMessage({
+        type: 'hot-hook:full-reload',
+        path: realFilePath,
+        shouldBeReloadable: shouldBeReloadable,
+      })
     }
 
     /**
@@ -223,10 +228,33 @@ export class HotHookLoader {
       const reloadable = context.importAttributes?.hot === 'true' ? true : isHardcodedBoundary
 
       if (reloadable) {
-        this.#dynamicImportChecker.ensureFileIsImportedDynamicallyFromParent(parentPath, specifier)
+        /**
+         * If supposed to be reloadable, we must ensure it is imported dynamically
+         * from the parent file. Otherwise, hot-hook can't invalidate the file
+         */
+        let isImportedDynamically =
+          await this.#dynamicImportChecker.ensureFileIsImportedDynamicallyFromParent(
+            parentPath,
+            specifier
+          )
+
+        /**
+         * Throw an error if not dynamically imported and the option is set
+         */
+        if (!isImportedDynamically && this.#options.throwWhenBoundariesAreNotDynamicallyImported)
+          throw new FileNotImportedDynamicallyException(parentPath, specifier, this.#projectRoot)
+
+        /**
+         * Otherwise, just add the file as not-reloadable ( so it will trigger a full reload )
+         */
+        this.#dependencyTree.addDependency(parentPath, {
+          path: resultPath,
+          reloadable: isImportedDynamically,
+          isWronglyImported: !isImportedDynamically,
+        })
+      } else {
+        this.#dependencyTree.addDependency(parentPath, { path: resultPath, reloadable })
       }
-
-      this.#dependencyTree.addDependency(parentPath, { path: resultPath, reloadable })
     }
 
     if (this.#pathIgnoredMatcher.match(resultPath)) {

packages/hot_hook/src/register.ts

@@ -14,5 +14,7 @@ const hotHookConfig = packageJson.hotHook
 await hot.init({
   ...(hotHookConfig || {}),
   rootDirectory: dirname(packageJsonPath),
+  throwWhenBoundariesAreNotDynamicallyImported:
+    hotHookConfig?.throwWhenBoundariesAreNotDynamicallyImported ?? false,
   root: hotHookConfig?.root ? resolve(packageJsonPath, hotHookConfig.root) : undefined,
 })

packages/hot_hook/src/types.ts

@@ -1,7 +1,7 @@
 import { MessagePort } from 'node:worker_threads'
 
 export type MessageChannelMessage =
-  | { type: 'hot-hook:full-reload'; path: string }
+  | { type: 'hot-hook:full-reload'; path: string; shouldBeReloadable?: boolean }
   | { type: 'hot-hook:invalidated'; paths: string[] }
 
 export interface InitOptions {
@@ -48,11 +48,22 @@ export interface InitOptions {
    * @default ['.env']
    */
   restart?: string[]
+
+  /**
+   * If true, the hook will throw an error if a boundary is not dynamically
+   * imported.
+   */
+  throwWhenBoundariesAreNotDynamicallyImported?: boolean
 }
 
 export type InitializeHookOptions = Pick<
   InitOptions,
-  'ignore' | 'root' | 'rootDirectory' | 'boundaries' | 'restart'
+  | 'ignore'
+  | 'root'
+  | 'rootDirectory'
+  | 'boundaries'
+  | 'restart'
+  | 'throwWhenBoundariesAreNotDynamicallyImported'
 > & {
   /**
    * The message port to communicate with the parent thread.

packages/hot_hook/tests/dependency_tree.spec.ts

@@ -21,14 +21,14 @@ test.group('Dependency tree', () => {
     tree.addDependency('controllers/posts_controller.ts', { path: 'models/post.ts' })
     tree.addDependency('models/post.ts', { path: 'services/post_service.ts' })
 
-    assert.deepEqual(tree.isReloadable('app.ts'), false)
-    assert.deepEqual(tree.isReloadable('start/index.ts'), false)
-    assert.deepEqual(tree.isReloadable('providers/database_provider.ts'), false)
-    assert.deepEqual(tree.isReloadable('controllers/users_controller.ts'), true)
-    assert.deepEqual(tree.isReloadable('controllers/posts_controller.ts'), true)
-    assert.deepEqual(tree.isReloadable('models/user.ts'), false)
-    assert.deepEqual(tree.isReloadable('models/post.ts'), true)
-    assert.deepEqual(tree.isReloadable('services/post_service.ts'), true)
+    assert.deepEqual(tree.isReloadable('app.ts').reloadable, false)
+    assert.deepEqual(tree.isReloadable('start/index.ts').reloadable, false)
+    assert.deepEqual(tree.isReloadable('providers/database_provider.ts').reloadable, false)
+    assert.deepEqual(tree.isReloadable('controllers/users_controller.ts').reloadable, true)
+    assert.deepEqual(tree.isReloadable('controllers/posts_controller.ts').reloadable, true)
+    assert.deepEqual(tree.isReloadable('models/user.ts').reloadable, false)
+    assert.deepEqual(tree.isReloadable('models/post.ts').reloadable, true)
+    assert.deepEqual(tree.isReloadable('services/post_service.ts').reloadable, true)
   })
 
   test('scenario 2', ({ assert }) => {
@@ -44,11 +44,11 @@ test.group('Dependency tree', () => {
     tree.addDependency('controllers/users_controller.ts', { path: 'user_presenter.ts' })
     tree.addDependency('user_presenter.ts', { path: 'utils/index.ts' })
 
-    assert.deepEqual(tree.isReloadable('app.ts'), false)
-    assert.deepEqual(tree.isReloadable('models/user.ts'), false)
-    assert.deepEqual(tree.isReloadable('start/index.ts'), false)
-    assert.deepEqual(tree.isReloadable('controllers/users_controller.ts'), true)
-    assert.deepEqual(tree.isReloadable('user_presenter.ts'), true)
+    assert.deepEqual(tree.isReloadable('app.ts').reloadable, false)
+    assert.deepEqual(tree.isReloadable('models/user.ts').reloadable, false)
+    assert.deepEqual(tree.isReloadable('start/index.ts').reloadable, false)
+    assert.deepEqual(tree.isReloadable('controllers/users_controller.ts').reloadable, true)
+    assert.deepEqual(tree.isReloadable('user_presenter.ts').reloadable, true)
 
     const invalidated = tree.invalidateFileAndDependents('user_presenter.ts')
 
@@ -72,7 +72,7 @@ test.group('Dependency tree', () => {
 
     tree.addDependency('controllers/users_controller.ts', { path: 'models/user.ts' })
 
-    assert.isFalse(tree.isReloadable('models/user.ts'))
+    assert.isFalse(tree.isReloadable('models/user.ts').reloadable)
     assert.deepEqual(tree.getVersion('models/user.ts'), 0)
 
     const invalidatedFiles = tree.invalidateFileAndDependents('controllers/users_controller.ts')