Support ESModule natively for config using internal API#6
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Pull request overview
This PR introduces native ESModule support to Hammerspoon 2 by leveraging JavaScriptCore's internal APIs through the InternalESModuleForSwiftJavaScriptCore framework. The implementation adds both synchronous and asynchronous ESModule evaluation capabilities to replace or supplement the existing require mechanism.
Key Changes:
- Added
MultiRootFSModuleLoaderclass implementingESModuleLoaderDelegateto handle ESModule resolution and loading across multiple filesystem roots - Introduced synchronous and asynchronous
evalFromURL()methods to theJSEngineclass for evaluating ESModules - Integrated ESModule support into the JSEngine initialization with automatic module loader delegation
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| JSEngine.swift | Added ESModule import, integrated MultiRootFSModuleLoader, and implemented both sync and async evalFromURL methods with promise handling |
| MultiRootFSModuleLoader.swift | New module loader implementing ESModuleLoaderDelegate with caching, multi-root resolution, and security checks for file system boundaries |
| JSEngineProtocol.swift | Updated protocol documentation to clarify async evalFromURL evaluates files as ES modules |
| MockJSEngine.swift | Extended mock implementation to support both synchronous and asynchronous evalFromURL overloads for testing |
Note: Based on my review, I was unable to identify the specific lines changed in the diff context to provide inline comments. However, I've identified several areas that would benefit from attention:
-
Test Coverage: The new
MultiRootFSModuleLoaderclass lacks dedicated unit tests, though the existing integration tests in the test suite may provide some coverage through the JSEngine tests. -
Return Value Inconsistency: In the async
evalFromURLmethod (JSEngine.swift, around line 138-140), the method returns the promise object itself rather than the resolved value after awaiting it, which may not align with the expected behavior. -
Empty Delegate Method: The
willEvaluateModulemethod in MultiRootFSModuleLoader (line 123-125) is implemented but empty, which may indicate incomplete functionality. -
Documentation: While the protocol documentation was updated, additional inline documentation explaining the module resolution algorithm and security boundaries would improve maintainability.
-
Future Work: As noted in the PR description, the current implementation only supports module paths starting with
/,./, or../, with plans to extend support for custom schemes using C++ APIs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hmm, this is both interesting and a little terrifying. As someone who isn't deep in the JS world, what would be the benefits of doing this vs require()? |
|
The current Pros
Cons
|
|
Thanks for the reply. As someone not super familiar with the JS ecosystem, it's somewhat hard for me to evaluate this, but I'll describe where my thoughts are right now: I'm concerned about taking on subtle, complex code at this stage of the project. I don't really know what the long-term options are going to be for using/installing third party JS code, and I'm not sure if it's a good idea to aim for something like Node compatibility or not. I think I'd like us to focus on the core functionality and architecture first, so we can match HS1 and start migrating users over. |
|
Whatever early-stage decision on module would be technical debts once prevailing. Module syntax has nothing to do with nodejs, we could support a module system without concerning npm or |
|
What if we just embed esbuild directly? 🤓👆 |
…ests Production fixes: - Move default IPC port to closure-scoped var to survive JSExport GC (cmsj#4) - Make AKLog synchronous on main thread so getConsole() sees entries immediately (cmsj#3) - Add rollback to cliInstall when man page symlink fails (cmsj#2) - Use Atomic<Int> for callDepth in HSMessagePort (#1) - Remove redundant Bundle.main.bundlePath optional casts (cmsj#8) - Escape control chars in REPL tab completion query (cmsj#9) - Deduplicate date format via HammerspoonLogEntry.formattedLine (cmsj#11) - Remove dead print replacement code in hs.ipc.js (cmsj#10) Test fixes: - Remove misleading hammerspoonProcess field in HS2CommandTests (cmsj#5) - Read pipes concurrently to prevent deadlock on large output (cmsj#6) - Replace unsafeBitCast with direct setObject in JSTestHarness (cmsj#7) - Save/restore evalHistory in console tests for isolation (cmsj#13) - Handle port name conflicts gracefully in default port tests (cmsj#12) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ests Production fixes: - Move default IPC port to closure-scoped var to survive JSExport GC (cmsj#4) - Make AKLog synchronous on main thread so getConsole() sees entries immediately (cmsj#3) - Add rollback to cliInstall when man page symlink fails (cmsj#2) - Use Atomic<Int> for callDepth in HSMessagePort (#1) - Remove redundant Bundle.main.bundlePath optional casts (cmsj#8) - Escape control chars in REPL tab completion query (cmsj#9) - Deduplicate date format via HammerspoonLogEntry.formattedLine (cmsj#11) - Remove dead print replacement code in hs.ipc.js (cmsj#10) Test fixes: - Remove misleading hammerspoonProcess field in HS2CommandTests (cmsj#5) - Read pipes concurrently to prevent deadlock on large output (cmsj#6) - Replace unsafeBitCast with direct setObject in JSTestHarness (cmsj#7) - Save/restore evalHistory in console tests for isolation (cmsj#13) - Handle port name conflicts gracefully in default port tests (cmsj#12) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Design artifacts from the grilling/domain-modeling session for the hs.calendar + hs.reminders + calendar-mcp Permission Proxy feature. Execution tracked at happyjake/vibecast#5 (slices cmsj#6-cmsj#11).
|
@ghostflyby I keep coming back to this PR and re-reading it and struggling to decide what to do... I'm working on a counter-proposal, a CommonJS module importer that doesn't need any JSC internals, so we can compare and contrast them. The list of missing functionality for a proper release of HS2 is shrinking by the day, so we do need to figure out what our solution is for both importing third party JS libraries, but also for distributing and importing HS2 spoons. I'd be very keen to have more discussions about this if you're still interested? As someone who hasn't lived and breathed the JS ecosystem, I would appreciate input from those who have! |
Facts in my opinion
Suggestions
|
|
Here is the CommonJS option: |
details
requireevalFromUrlSome tests just fails with nothing to do with my changes.
demo
2025-12-28.22.37.59.mov
future works
unfortunately, the ESModule only supports ones starting with
/,./and../according to https://github.com/WebKit/WebKit/blob/8615f506db9162ea73173834af9e4b997d92dd93/Source/JavaScriptCore/API/JSAPIGlobalObject.mm#L94To support custom schemes or modulemap like features for introducing npm, I'll have to use internal cpp based APIs instead of ObjC-based ones.