From aeda2c2f8fe148b1380551248a824d422253471e Mon Sep 17 00:00:00 2001 From: Julien Date: Mon, 28 Oct 2024 22:23:17 +0100 Subject: [PATCH] feat: full-reload when boundary file is not dynamically imported --- .changeset/blue-taxis-poke.md | 17 +++ packages/hot_hook/src/dependency_tree.ts | 44 ++++-- .../hot_hook/src/dynamic_import_checker.ts | 12 +- ...file_not_imported_dynamically_exception.ts | 0 packages/hot_hook/src/hot.ts | 9 +- packages/hot_hook/src/loader.ts | 48 +++--- packages/hot_hook/src/register.ts | 2 + packages/hot_hook/src/types.ts | 15 +- .../hot_hook/tests/dependency_tree.spec.ts | 28 ++-- .../tests/dynamic_import_checker.spec.ts | 12 +- packages/hot_hook/tests/helpers.ts | 4 + packages/hot_hook/tests/loader.spec.ts | 138 ++++++++++++++++++ 12 files changed, 262 insertions(+), 67 deletions(-) create mode 100644 .changeset/blue-taxis-poke.md rename packages/hot_hook/src/{ => errors}/file_not_imported_dynamically_exception.ts (100%) diff --git a/.changeset/blue-taxis-poke.md b/.changeset/blue-taxis-poke.md new file mode 100644 index 0000000..3ac763f --- /dev/null +++ b/.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 + } +} +``` diff --git a/packages/hot_hook/src/dependency_tree.ts b/packages/hot_hook/src/dependency_tree.ts index d32e5e1..18c7f65 100644 --- a/packages/hot_hook/src/dependency_tree.ts +++ b/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 = new Set()): boolean => { + const checkPathToRoot = ( + currentNode: FileNode, + visited: Set = 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, })) } } diff --git a/packages/hot_hook/src/dynamic_import_checker.ts b/packages/hot_hook/src/dynamic_import_checker.ts index 5a0c454..87d1793 100644 --- a/packages/hot_hook/src/dynamic_import_checker.ts +++ b/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> = 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 } diff --git a/packages/hot_hook/src/file_not_imported_dynamically_exception.ts b/packages/hot_hook/src/errors/file_not_imported_dynamically_exception.ts similarity index 100% rename from packages/hot_hook/src/file_not_imported_dynamically_exception.ts rename to packages/hot_hook/src/errors/file_not_imported_dynamically_exception.ts diff --git a/packages/hot_hook/src/hot.ts b/packages/hot_hook/src/hot.ts index 65850af..f26bdf9 100644 --- a/packages/hot_hook/src/hot.ts +++ b/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, }) diff --git a/packages/hot_hook/src/loader.ts b/packages/hot_hook/src/loader.ts index 71e5096..bad638b 100644 --- a/packages/hot_hook/src/loader.ts +++ b/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)) { diff --git a/packages/hot_hook/src/register.ts b/packages/hot_hook/src/register.ts index 1fcd42d..ae187dd 100644 --- a/packages/hot_hook/src/register.ts +++ b/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, }) diff --git a/packages/hot_hook/src/types.ts b/packages/hot_hook/src/types.ts index a8e8882..3151191 100644 --- a/packages/hot_hook/src/types.ts +++ b/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. diff --git a/packages/hot_hook/tests/dependency_tree.spec.ts b/packages/hot_hook/tests/dependency_tree.spec.ts index 076e00d..2fb7461 100644 --- a/packages/hot_hook/tests/dependency_tree.spec.ts +++ b/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') diff --git a/packages/hot_hook/tests/dynamic_import_checker.spec.ts b/packages/hot_hook/tests/dynamic_import_checker.spec.ts index f36cf09..f16a13b 100644 --- a/packages/hot_hook/tests/dynamic_import_checker.spec.ts +++ b/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')) diff --git a/packages/hot_hook/tests/helpers.ts b/packages/hot_hook/tests/helpers.ts index e0c0c1f..7e9c71c 100644 --- a/packages/hot_hook/tests/helpers.ts +++ b/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 + }, } } diff --git a/packages/hot_hook/tests/loader.spec.ts b/packages/hot_hook/tests/loader.spec.ts index bc1ab59..059c054 100644 --- a/packages/hot_hook/tests/loader.spec.ts +++ b/packages/hot_hook/tests/loader.spec.ts @@ -315,4 +315,142 @@ test.group('Loader', () => { ) assert.isDefined(result) }).disableTimeout() + + test('full reload if file should be reloadable but is not dynamically imported', async ({ + fs, + assert, + }) => { + await fakeInstall(fs.basePath) + + await fs.createJson('package.json', { type: 'module', hotHook: { boundaries: ['./app.js'] } }) + await fs.create( + 'server.js', + `import * as http from 'http' + import { hot } from 'hot-hook' + import { join } from 'node:path' + import app from './app.js' + + const server = http.createServer(async (request, response) => { + await app(request, response) + }) + + server.listen(3333, () => { + console.log('Server is running') + })` + ) + + await createHandlerFile({ path: 'app.js', response: 'Hello World!' }) + + const server = runProcess('server.js', { + cwd: fs.basePath, + env: { NODE_DEBUG: 'hot-hook' }, + nodeOptions: ['--import=hot-hook/register'], + }) + + await server.waitForOutput('Server is running') + await supertest('http://localhost:3333').get('/').expect(200).expect('Hello World!') + + await createHandlerFile({ path: 'app.js', response: 'Hello World! Updated' }) + await setTimeout(100) + + const result = await pEvent( + server.child, + 'message', + (message: any) => + message?.type === 'hot-hook:full-reload' && message.shouldBeReloadable === true + ) + assert.isDefined(result) + }).disableTimeout() + + test('send shouldBeReloadable if parent boundary is not dynamically importd', async ({ + fs, + assert, + }) => { + await fakeInstall(fs.basePath) + + await fs.createJson('package.json', { type: 'module', hotHook: { boundaries: ['./app.js'] } }) + await fs.create( + 'server.js', + `import * as http from 'http' + import { hot } from 'hot-hook' + import { join } from 'node:path' + import app from './app.js' + + const server = http.createServer(async (request, response) => { + await app(request, response) + }) + + server.listen(3333, () => { + console.log('Server is running') + })` + ) + + await fs.create( + 'app.js', + ` + import { test } from './app2.js' + + export default function(request, response) { + response.writeHead(200, {'Content-Type': 'text/plain'}) + response.end('Hello World!') + }` + ) + await fs.create(`app2.js`, `export function test() { return 'Hello World!' }`) + + const server = runProcess('server.js', { + cwd: fs.basePath, + env: { NODE_DEBUG: 'hot-hook' }, + nodeOptions: ['--import=hot-hook/register'], + }) + + await server.waitForOutput('Server is running') + await supertest('http://localhost:3333').get('/').expect(200).expect('Hello World!') + + await fs.create(`app2.js`, `export function test() { return 'Hello Test!' }`) + + await setTimeout(100) + + const result = await pEvent(server.child, 'message', (message: any) => { + console.log(message) + return message?.type === 'hot-hook:full-reload' && message.shouldBeReloadable === true + }) + assert.isDefined(result) + }).disableTimeout() + + test('throw error if file should be reloadable but is not dynamically imported and flag is set', async ({ + fs, + assert, + }) => { + await fakeInstall(fs.basePath) + + await fs.createJson('package.json', { + type: 'module', + hotHook: { boundaries: ['./app.js'], throwWhenBoundariesAreNotDynamicallyImported: true }, + }) + await fs.create( + 'server.js', + `import * as http from 'http' + import { hot } from 'hot-hook' + import { join } from 'node:path' + import app from './app.js' + + const server = http.createServer(async (request, response) => { + await app(request, response) + }) + + server.listen(3333, () => { + console.log('Server is running') + })` + ) + + await createHandlerFile({ path: 'app.js', response: 'Hello World!' }) + + const server = runProcess('server.js', { + cwd: fs.basePath, + env: { NODE_DEBUG: 'hot-hook' }, + nodeOptions: ['--import=hot-hook/register'], + }) + + await assert.rejects(async () => await server.child!) + }).disableTimeout() })