👍 Remove Worker from the system (POC)
#291
Conversation
WalkthroughThe changes across various TypeScript files in the codebase reflect a significant refactoring effort. The Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- denops/@denops-private/host/base.ts (1 hunks)
- denops/@denops-private/host/nvim.ts (1 hunks)
- denops/@denops-private/host/vim.ts (1 hunks)
- denops/@denops-private/impl.ts (2 hunks)
- denops/@denops-private/service.ts (6 hunks)
Additional comments: 10
denops/@denops-private/host/base.ts (1)
- 18-23:
The return type of thebatchmethod is[unknown[], string], which suggests it returns a single tuple. Verify if this is the intended behavior or if it should return an array of tuples, each corresponding to the result of a batch call.denops/@denops-private/host/nvim.ts (1)
- 63-67:
The changes to thebatchmethod are consistent with the pull request summary and follow TypeScript best practices. The use of a rest parameter with tuple types ensures type safety and immutability of the input arguments. The method correctly maps the calls to the expected format for 'nvim_call_atomic' and handles errors appropriately.denops/@denops-private/host/vim.ts (1)
- 39-43:
The change to thebatchmethod signature to use a rest parameter of type(readonly [string, ...unknown[]])[]is consistent with the goal of enforcing immutability. Ensure that all calls to this method throughout the codebase are updated to pass immutable arrays.denops/@denops-private/impl.ts (3)
5-16:
Verify if thedispatcherproperty should be public or if it was intended to be private or protected.35-44:
The changes to thebatchmethod correctly handle the new tuple type arguments and maintain consistent error handling.51-68:
Thedispatchmethod is correctly updated to interact with theServiceinstance and is appropriately marked as async.denops/@denops-private/service.ts (4)
26-30:
The_traceparameter is not used in theregistermethod. If this is intentional and reserved for future use, consider documenting its purpose to avoid confusion.45-45:
The error handling for already registered plugins is clear and informative.115-120:
The error handling in thedispatchmethod is consistent with the requirements of Vim/Neovim, which do not handle JavaScript Error instances.126-128:
Thedisposemethod is currently a no-op. If this method is not intended to be implemented in the future, consider removing it to reduce confusion. If it is part of theDisposableinterface or reserved for future use, document its current state and intended purpose.
| const denops = new DenopsImpl(name, meta, this); | ||
| // Import module with fragment so that reload works properly | ||
| // https://github.com/vim-denops/denops.vim/issues/227 | ||
| const suffix = `#${performance.now()}`; | ||
| const scriptUrl = resolveScriptUrl(script); | ||
| worker.postMessage({ scriptUrl: `${scriptUrl}${suffix}`, meta, trace }); | ||
| const session = buildServiceSession( | ||
| name, | ||
| meta, | ||
| readableStreamFromWorker(worker), | ||
| writableStreamFromWorker(worker), | ||
| this, | ||
| trace, | ||
| ); | ||
| import(`${scriptUrl}${suffix}`).then(async (mod) => { | ||
| try { | ||
| await denops.cmd( | ||
| `doautocmd <nomodeline> User DenopsSystemPluginPre:${name}`, | ||
| ) | ||
| .catch((e) => | ||
| console.warn( | ||
| `Failed to emit DenopsSystemPluginPre:${name}: ${e}`, | ||
| ) | ||
| ); | ||
| await mod.main(denops); | ||
| await denops.cmd( | ||
| `doautocmd <nomodeline> User DenopsSystemPluginPost:${name}`, | ||
| ) | ||
| .catch((e) => | ||
| console.warn( | ||
| `Failed to emit DenopsSystemPluginPost:${name}: ${e}`, | ||
| ) | ||
| ); | ||
| } catch (e) { | ||
| console.error(e); | ||
| await denops.cmd( | ||
| `doautocmd <nomodeline> User DenopsSystemPluginFail:${name}`, | ||
| ) | ||
| .catch((e) => | ||
| console.warn( | ||
| `Failed to emit DenopsSystemPluginFail:${name}: ${e}`, | ||
| ) | ||
| ); | ||
| } | ||
| }); | ||
| this.#plugins.set(name, { | ||
| script, | ||
| worker, | ||
| session, | ||
| client: new Client(session, { errorDeserializer }), | ||
| denops, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Consider improving error handling within the dynamic import. Logging to the console may not be sufficient for production environments. It might be beneficial to implement a more robust error reporting mechanism or to rethrow the error after logging.
|
See #293 |
TBW
Summary by CodeRabbit
Refactor
batchfunction across various classes for improved type handling.Serviceinstance, enhancing the plugin's architecture.Serviceclass by integratingDenopsImpland simplifying method calls.Chores
Sessiontype and associated properties to clean up the codebase.disposemethod in theServiceclass.