feat: full-reload when boundary file is not dynamically imported

.changeset/blue-taxis-poke.md

@@ -0,0 +1,17 @@
+---
+'hot-hook': minor
+---
+
+Plus tot, avec cette release https://github.com/Julien-R44/hot-hook/releases/tag/hot-hook%400.3.0 on se contentait de throw et donc de killer l'app quand un fichier boundary n'était pas importé dynamiquement car cela empechait à hot-hook de hot-reloader. 
+
+Maintenant, on ne throw plus par défault l'erreur, on emet simplement un message de type "hot-hook:full-reload" au process parent, qui lui sera chargé de restart l'app entière. Ce message "hot-hook:full-reload" n'est pas nouveau, il était déjà utilisé pour les fichiers qui devaient trigger un full reload. 
+
+Si jamais vous souhaitez quand meme throw l'erreur, alors vous pouvez passer l'options `throwWhenBoundariesAreNotDynamicallyImported` à true, quand vous appellez `hot.init` ou dans votre `package.json` : 
+
+```json
+{
+  "hotHook": {
+    "throwWhenBoundariesAreNotDynamicallyImported": true
+  }
+}
+```

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 { FileNotImportedDynamicallyException } from './file_not_imported_dynamically_exception.js'
 
 /**
  * This class is responsible for checking if a given specifier
@@ -11,16 +10,11 @@ import { FileNotImportedDynamicallyException } from './file_not_imported_dynamic
  */
 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,10 +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 FileNotImportedDynamicallyException(parentPath, specifier, this.projectRoot)
-    }
-
     return isFileDynamicallyImportedFromParent
   }
 

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,7 +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 './file_not_imported_dynamically_exception.js'
+import { FileNotImportedDynamicallyException } from './errors/file_not_imported_dynamically_exception.js'
 
 export class HotHookLoader {
   #options: InitializeHookOptions
@@ -31,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))
   }
 
@@ -105,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,
+      })
     }
 
     /**
@@ -216,23 +220,33 @@ export class HotHookLoader {
       const reloadable = context.importAttributes?.hot === 'true' ? true : isHardcodedBoundary
 
       if (reloadable) {
-        try {
+        /**
+         * 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
           )
-        } catch (e) {
-          if (e instanceof FileNotImportedDynamicallyException) {
-            debug('File not imported dynamically %s', resultPath)
-            this.#dependencyTree.addDependency(parentPath, { path: resultPath, reloadable: false })
-            return result
-          }
-
-          throw e
-        }
-      }
 
-      this.#dependencyTree.addDependency(parentPath, { path: resultPath, reloadable })
+        /**
+         * 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 })
+      }
     }
 
     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')

packages/hot_hook/tests/dynamic_import_checker.spec.ts

@@ -15,17 +15,11 @@ test.group('Dynamic Import Checker', () => {
       `
     )
 
-    const checker = new DynamicImportChecker(fs.basePath)
-
+    const checker = new DynamicImportChecker()
     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.isFalse(await checker.ensureFileIsImportedDynamicallyFromParent(path, './foo'))
+    assert.isFalse(await checker.ensureFileIsImportedDynamicallyFromParent(path, '#app/aliases'))
 
     assert.isTrue(await checker.ensureFileIsImportedDynamicallyFromParent(path, './bla'))
     assert.isTrue(await checker.ensureFileIsImportedDynamicallyFromParent(path, '#app/aliases-bla'))

packages/hot_hook/tests/helpers.ts

@@ -70,5 +70,9 @@ export function runProcess(scriptPath: string, options?: NodeOptions) {
         message: `Timeout waiting for "${output}"`,
       })
     },
+
+    async waitForExit() {
+      await child
+    },
   }
 }