-
-
Notifications
You must be signed in to change notification settings - Fork 208
feat: resolve ESM imports of host-provided modules to the server's copy #2958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dirkwa
wants to merge
5
commits into
SignalK:master
Choose a base branch
from
dirkwa:host-modules-esm-hook
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+272
−31
Open
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
dcc78ea
feat: resolve core modules to the server's copy
48db5b8
chore: re-trigger CI
c8679da
refactor: mark resolve hook options as optional
197a042
feat: cover ESM import of host-provided modules
6f9d512
test: gate ESM fixture on registerHooks support
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /* | ||
| * Host-provided modules | ||
| * | ||
| * Plugins installed in the server's data directory resolve their | ||
| * dependencies from <configPath>/node_modules, never from the server's own | ||
| * tree. For core interface packages this is harmful: dual copies break | ||
| * object identity between plugin and server code, and one plugin's tight | ||
| * version pin holds the hoisted copy back for every other plugin that would | ||
| * accept a newer version (npm optimizes for dedupe, not freshness). | ||
| * | ||
| * This module hooks Node's module resolution so that loading any of the | ||
| * packages listed below — including exported subpaths such as | ||
| * '@signalk/server-api/history' — always resolves to the server's own copy, | ||
| * no matter what npm installed in the plugin tree: the same model as | ||
| * require('vscode') in VS Code extensions. Plugin-bundled copies remain on | ||
| * disk but are never loaded. | ||
| * | ||
| * Coverage comes from two hooks: the CJS _resolveFilename hook answers | ||
| * require() on every supported Node version, and module.registerHooks | ||
| * (Node >= 22.15) extends the same redirect to `import` statements in ESM | ||
| * plugins, which never reach the CJS hook. On Node 22.0-22.14 only the | ||
| * CJS hook is available, so a pure-ESM plugin bundling its own copy can | ||
| * still load it there. | ||
| * | ||
| * This module MUST be imported before any other module that may load one of | ||
| * the host-provided packages. | ||
| */ | ||
|
|
||
| import Module from 'module' | ||
| import { pathToFileURL } from 'url' | ||
| import { createDebug } from './debug' | ||
|
|
||
| const debug = createDebug('signalk-server:host-modules') | ||
|
|
||
| const HOST_PROVIDED_MODULES = ['baconjs', '@signalk/server-api'] | ||
|
|
||
| const hostModulePaths = new Map<string, string | null>( | ||
| HOST_PROVIDED_MODULES.map((name) => [name, require.resolve(name)]) | ||
| ) | ||
|
|
||
| function isHostProvided(request: string): boolean { | ||
| return HOST_PROVIDED_MODULES.some( | ||
| (name) => request === name || request.startsWith(name + '/') | ||
| ) | ||
| } | ||
|
|
||
| function resolveHostPath(request: string): string | null { | ||
| let hostPath = hostModulePaths.get(request) | ||
| if (hostPath === undefined) { | ||
| try { | ||
| // resolve from this module's context via the original resolver: | ||
| // require.resolve() would re-enter the hook and recurse | ||
| hostPath = origResolveFilename.call(Module, request, module, false) | ||
| } catch { | ||
| // subpath not exported by the host copy: leave resolution alone, so | ||
| // requires that only work against a bundled copy keep working | ||
| hostPath = null | ||
| } | ||
| hostModulePaths.set(request, hostPath) | ||
| } | ||
| return hostPath | ||
| } | ||
|
|
||
| type ResolveFilename = ( | ||
| request: string, | ||
| parent: NodeModule | undefined, | ||
| isMain: boolean, | ||
| options?: Record<string, unknown> | ||
| ) => string | ||
|
|
||
| const ModuleInternal = Module as unknown as Record<string, unknown> | ||
|
|
||
| const origResolveFilename = ModuleInternal._resolveFilename as ResolveFilename | ||
| ModuleInternal._resolveFilename = function ( | ||
| request: string, | ||
| parent: NodeModule | undefined, | ||
| isMain: boolean, | ||
| options?: Record<string, unknown> | ||
| ) { | ||
| if (isHostProvided(request)) { | ||
| const hostPath = resolveHostPath(request) | ||
| if (hostPath !== null) { | ||
| debug.enabled && | ||
| debug(`resolving ${request} for ${parent?.filename} to the host copy`) | ||
| return hostPath | ||
| } | ||
| } | ||
| return origResolveFilename.call(this, request, parent, isMain, options) | ||
| } | ||
|
|
||
| // registerHooks resolve hooks see both `import` and require() specifiers, | ||
| // but never the direct origResolveFilename calls made by resolveHostPath — | ||
| // verified empirically, so no re-entrancy guard is needed here | ||
| if (typeof Module.registerHooks === 'function') { | ||
| Module.registerHooks({ | ||
| resolve(specifier, context, nextResolve) { | ||
| if (isHostProvided(specifier)) { | ||
| const hostPath = resolveHostPath(specifier) | ||
| if (hostPath !== null) { | ||
| debug.enabled && | ||
| debug( | ||
| `resolving ${specifier} for ${context.parentURL} to the host copy` | ||
| ) | ||
| return { url: pathToFileURL(hostPath).href, shortCircuit: true } | ||
| } | ||
| } | ||
| return nextResolve(specifier, context) | ||
| } | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| */ | ||
|
|
||
| import './networkConfig' | ||
| import './host-modules' | ||
| import './baconjs-compat' | ||
| import { | ||
| Context, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| import { expect } from 'chai' | ||
| import fs from 'fs' | ||
| import Module from 'module' | ||
| import os from 'os' | ||
| import path from 'path' | ||
| import '../dist/host-modules.js' | ||
| import { importOrRequire } from '../dist/modules.js' | ||
|
|
||
| // ESM import coverage needs module.registerHooks (Node >= 22.15); on older | ||
| // Node 22.x the server intentionally falls back to CJS-only coverage | ||
| const itWithRegisterHooks = | ||
| typeof Module.registerHooks === 'function' ? it : it.skip | ||
|
|
||
| function writeModule(dir: string, pkg: object, files: Record<string, string>) { | ||
| fs.mkdirSync(dir, { recursive: true }) | ||
| fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify(pkg)) | ||
| for (const [name, content] of Object.entries(files)) { | ||
| fs.writeFileSync(path.join(dir, name), content) | ||
| } | ||
| } | ||
|
|
||
| describe('host-provided modules', () => { | ||
| let testDir: string | undefined | ||
| let plugin: { | ||
| serverApi: Record<string, unknown> | ||
| historyApi: Record<string, unknown> | ||
| deep: { BUNDLED_STALE_COPY?: boolean } | ||
| bacon: Record<string, unknown> | ||
| } | ||
| let esmPlugin: typeof plugin | ||
|
|
||
| before(async () => { | ||
| testDir = fs.mkdtempSync( | ||
| path.join(os.tmpdir(), '_skservertest_host_modules') | ||
| ) | ||
| const pluginDir = path.join(testDir, 'node_modules', 'testplugin') | ||
| writeModule( | ||
| pluginDir, | ||
| { name: 'testplugin', version: '1.0.0', main: 'index.js' }, | ||
| { | ||
| 'index.js': `module.exports = { | ||
| serverApi: require('@signalk/server-api'), | ||
| historyApi: require('@signalk/server-api/history'), | ||
| deep: require('@signalk/server-api/deep.js'), | ||
| bacon: require('baconjs') | ||
| }` | ||
| } | ||
| ) | ||
| writeStaleBundledCopies(pluginDir) | ||
| plugin = await importOrRequire(pluginDir) | ||
|
|
||
| const esmPluginDir = path.join(testDir, 'node_modules', 'testplugin-esm') | ||
| writeModule( | ||
| esmPluginDir, | ||
| { | ||
| name: 'testplugin-esm', | ||
| version: '1.0.0', | ||
| type: 'module', | ||
| main: 'index.js' | ||
| }, | ||
| { | ||
| 'index.js': `import serverApi from '@signalk/server-api' | ||
| import historyApi from '@signalk/server-api/history' | ||
| import deep from '@signalk/server-api/deep.js' | ||
| import bacon from 'baconjs' | ||
| export { serverApi, historyApi, deep, bacon }` | ||
| } | ||
| ) | ||
| writeStaleBundledCopies(esmPluginDir) | ||
| esmPlugin = await importOrRequire(esmPluginDir) | ||
| }) | ||
|
|
||
| // stale bundled copies without an exports map, like server-api 2.9.x | ||
| function writeStaleBundledCopies(pluginDir: string) { | ||
| writeModule( | ||
| path.join(pluginDir, 'node_modules', '@signalk', 'server-api'), | ||
| { name: '@signalk/server-api', version: '0.0.1', main: 'index.js' }, | ||
| { | ||
| 'index.js': `module.exports = { BUNDLED_STALE_COPY: true }`, | ||
| 'history.js': `module.exports = { BUNDLED_STALE_COPY: true }`, | ||
| 'deep.js': `module.exports = { BUNDLED_STALE_COPY: true }` | ||
| } | ||
| ) | ||
| writeModule( | ||
| path.join(pluginDir, 'node_modules', 'baconjs'), | ||
| { name: 'baconjs', version: '0.0.1', main: 'index.js' }, | ||
| { 'index.js': `module.exports = { BUNDLED_STALE_COPY: true }` } | ||
| ) | ||
| } | ||
|
|
||
| after(() => { | ||
| if (testDir !== undefined) { | ||
| fs.rmSync(testDir, { recursive: true, force: true }) | ||
| } | ||
| }) | ||
|
|
||
| it('plugin bundling its own @signalk/server-api gets the host copy', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| expect(plugin.serverApi).to.equal(require('@signalk/server-api')) | ||
| expect(plugin.serverApi.BUNDLED_STALE_COPY).to.equal(undefined) | ||
| }) | ||
|
|
||
| it('exported subpaths resolve to the host copy', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| expect(plugin.historyApi).to.equal(require('@signalk/server-api/history')) | ||
| expect(plugin.historyApi.BUNDLED_STALE_COPY).to.equal(undefined) | ||
| }) | ||
|
|
||
| it('subpaths the host copy does not export resolve normally', () => { | ||
| expect(plugin.deep.BUNDLED_STALE_COPY).to.equal(true) | ||
| }) | ||
|
|
||
| it('plugin bundling its own baconjs gets the host copy', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| expect(plugin.bacon).to.equal(require('baconjs')) | ||
| expect(plugin.bacon.BUNDLED_STALE_COPY).to.equal(undefined) | ||
| }) | ||
|
|
||
| itWithRegisterHooks( | ||
| 'ESM plugin importing @signalk/server-api gets the host copy', | ||
| () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| expect(esmPlugin.serverApi).to.equal(require('@signalk/server-api')) | ||
| expect(esmPlugin.serverApi.BUNDLED_STALE_COPY).to.equal(undefined) | ||
| } | ||
| ) | ||
|
|
||
| itWithRegisterHooks( | ||
| 'ESM-imported exported subpaths resolve to the host copy', | ||
| () => { | ||
| expect(esmPlugin.historyApi).to.equal( | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| require('@signalk/server-api/history') | ||
| ) | ||
| expect(esmPlugin.historyApi.BUNDLED_STALE_COPY).to.equal(undefined) | ||
| } | ||
| ) | ||
|
|
||
| itWithRegisterHooks( | ||
| 'ESM-imported subpaths the host copy does not export resolve normally', | ||
| () => { | ||
| expect(esmPlugin.deep.BUNDLED_STALE_COPY).to.equal(true) | ||
| } | ||
| ) | ||
|
|
||
| itWithRegisterHooks('ESM plugin importing baconjs gets the host copy', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| expect(esmPlugin.bacon).to.equal(require('baconjs')) | ||
| expect(esmPlugin.bacon.BUNDLED_STALE_COPY).to.equal(undefined) | ||
| }) | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.