💥 Minor breaking changes #300
Conversation
WalkthroughThe updates across various Vim script and TypeScript files in Denops reflect a shift in plugin handling and message dispatching. The changes streamline the flow by centralizing meta information and refining control mechanisms. This reorganization suggests a move towards more efficient and maintainable code, particularly in the way plugins register and reload, and how the system handles internal notifications and service sessions. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
=======================================
Coverage 96.47% 96.47%
=======================================
Files 3 3
Lines 85 85
Branches 6 6
=======================================
Hits 82 82
Misses 3 3 ☔ View full report in Codecov by Sentry. |
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- autoload/denops/_internal/server/chan.vim (1 hunks)
- autoload/denops/plugin.vim (2 hunks)
- denops/@denops-private/cli.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (3 hunks)
- denops/@denops-private/host/vim.ts (4 hunks)
- denops/@denops-private/service.ts (5 hunks)
- doc/denops.txt (1 hunks)
Additional comments: 22
denops/@denops-private/cli.ts (2)
8-8: The addition of the
isMetaimport from./util.tsis noted. Ensure that theisMetafunction is used appropriately in the context of the changes.40-43: The introduction of the
metavariable and the use ofensurewithisMetato validate themetaobject is a good practice to ensure type safety. This change aligns with the PR's objective to readmetaonly once during initialization.denops/@denops-private/host/invoker.ts (2)
38-38: The removal of the
metaparameter from theregistermethod simplifies the method signature and aligns with the PR's objective to use a singlemetainstance.46-46: Similarly, the removal of the
metaparameter from thereloadmethod is consistent with the changes made to theregistermethod and the PR's objectives.denops/@denops-private/host/vim.ts (4)
14-14: The addition of the
isVoidMessageconstant is a clear way to identify void messages, which seems to be a new concept introduced in this PR.34-45: The changes in the constructor to handle messages and dispatch them accordingly are significant. It's important to ensure that the
#invokeris properly initialized before any messages are dispatched to avoid runtime errors.77-77: Assigning the
invokerproperty in theregistermethod is a straightforward change that supports the new message handling logic.93-102: The
dispatchfunction's refinement to handle void messages and check for a registered invoker is a critical change. It's important to ensure that theinvokeris notundefinedwhendispatchis called.denops/@denops-private/host/nvim.ts (3)
28-28: The addition of the
#invokerfield to theNeovimclass is consistent with the changes made to theVimclass and supports the new message handling logic.37-54: The modifications in the constructor to use the assigned
#invokerfor method invocation are significant. It's important to ensure that the#invokeris properly initialized before any invocations are made.112-112: The change to the
registermethod to assign theinvokerto#invokeris a straightforward update that aligns with the PR's objectives.autoload/denops/_internal/server/chan.vim (1)
- 100-100: Replacing the direct
doautocmdcall withs:rpcnotifyalters the signaling mechanism, which should ensure that theDenopsReadyevent is fired at the correct time, as per the PR's objectives.denops/@denops-private/service.ts (5)
30-37: The inclusion of the
readonly metaproperty in theServiceclass constructor is a key change that supports the PR's objective to use a singlemetainstance throughout the service's lifecycle.49-56: The update to the
registermethod to usethis.metainstead of passingmetaas an argument simplifies the method signature and is consistent with the PR's objectives.75-79: The postMessage to the worker now includes
metaandtracedirectly, which is a change that should be verified to ensure that the worker script is expecting these values and handling them correctly.Verification successful
The shell script results confirm that the
worker/script.tsfile contains an event listener for messages that deconstructs the event data to obtainscriptUrl,meta, andtrace. This indicates that the worker script is indeed expecting these values and has logic in place to handle them.Based on the provided output, the following can be inferred:
- The
addEventListenerformessageevents is set up to handle incoming data that includesmetaandtrace.- The
ensurefunction is used to validate the structure of the incoming message, which includesmetaandtrace.- The
mainfunction is called withscriptUrl,meta, andtraceas arguments, withtracehaving a fallback tofalseif it is not provided.Given this information, the review comment concerning the verification of the worker script's handling of
metaandtracehas been addressed, and it appears that the worker script is correctly handling these values.* 103-111: The update to the `reload` method to use `this.meta` instead of passing `meta` as an argument is consistent with the changes made to the `register` method and supports the PR's objectives.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the worker script is correctly handling the `meta` and `trace` values passed in the postMessage. rg --type typescript --files-with-matches 'addEventListener.*message.*meta.*trace'Length of output: 119
Script:
#!/bin/bash # Search for the event listener in .ts files that handles messages containing 'meta' and 'trace' rg 'addEventListener.*message' -g '*.ts' | rg 'meta' | rg 'trace'Length of output: 63
Script:
#!/bin/bash # Search for occurrences of 'meta' and 'trace' within the scope of message event handling in .ts files rg 'addEventListener.*message' -g '*.ts' -C 10 | rg 'meta' rg 'addEventListener.*message' -g '*.ts' -C 10 | rg 'trace'Length of output: 532
- 163-163: The
reloaddispatcher function now callsservice.reloadwith theskipmode and thetraceargument. This change should be verified to ensure that it aligns with the expected behavior when a plugin is reloaded.autoload/denops/plugin.vim (4)
64-70: The refactoring of
denops#plugin#registerremoves thedenops#_internal#meta#get()call and simplifies the handling of thel:optionsdictionary. Ensure that the removal ofmetafetching does not affect any dependent functionality and that thel:optionsdictionary is correctly populated with the necessary default values.73-82: The
denops#plugin#reloadfunction has been updated to remove themetaparameter. Verify that the newl:argsarray, which no longer includesmeta, is correctly used in thedenops#_internal#server#chan#notifycall and that all dependent features work as expected without themetainformation.84-93: The
denops#plugin#discoverfunction has been refactored to remove themetaparameter. Confirm that thes:registercall within the loop correctly handles plugin registration without themetainformation and that the defaultl:optionsvalues are appropriate for the discovery process.134-141: The
s:registerfunction has been updated to remove themetaparameter. Ensure that thel:argsarray is correctly constructed for thedenops#_internal#server#chan#notifycall and that the removal ofmetadoes not negatively impact the plugin registration process.doc/denops.txt (1)
- 90-96: The documentation has been updated to include the new requirement that
g:denops#debugmust be configured before the denops initialization. This is a breaking change and should be clearly communicated to users. Ensure that the documentation is clear and that there are no other sections that contradict this new requirement.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- autoload/denops.vim (1 hunks)
- autoload/denops/plugin.vim (4 hunks)
- autoload/denops/server.vim (5 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/service.ts (6 hunks)
- denops/@denops-private/worker/script.ts (4 hunks)
- doc/denops.txt (6 hunks)
- plugin/denops.vim (2 hunks)
Files skipped from review due to trivial changes (1)
- denops/@denops-private/worker/script.ts
Files skipped from review as they are similar to previous changes (4)
- autoload/denops/plugin.vim
- denops/@denops-private/host/invoker.ts
- denops/@denops-private/service.ts
- doc/denops.txt
Additional comments: 10
plugin/denops.vim (2)
19-24: The autocmds for
DenopsPluginPre,DenopsPluginPost, andDenopsPluginFailhave been removed. Ensure that their removal is intentional and that no functionality is lost or needs to be replicated elsewhere.35-35: The addition of
denops#server#wait_asyncwithins:init()is aligned with the PR's objective to improve the initialization process. Ensure that thedenops#plugin#discover()callback is correctly implemented and that it will be called once the server is ready.autoload/denops.vim (1)
- 30-34: The configuration for
denops#tracehas been removed. Confirm that this change is consistent with the PR's objectives and that any functionality dependent on this configuration has been appropriately updated or removed.autoload/denops/server.vim (7)
3-7: The introduction of
s:STATUS_PREPARING,s:is_ready, ands:ready_callbacksis consistent with the PR's goal to improve the plugin's initialization process. Verify that these new variables are used correctly throughout the codebase.74-74: The modification of
denops#server#status()to returns:STATUS_PREPARINGwhens:is_readyis false is a logical change that aligns with the new server status management. Ensure that all calls todenops#server#status()handle the new possible return value correctly.81-113: The addition of
denops#server#waitfunction is a significant change. Ensure that the function's implementation correctly handles the waiting logic, including the timeout and silent options.115-121: The
denops#server#wait_asyncfunction is added to handle asynchronous waiting for server readiness. Confirm that the callback mechanism is correctly implemented and that it integrates well with the rest of the codebase.135-140: The
s:DenopsReadyfunction correctly setss:is_readyto true and executes all callbacks. Verify that this function is called at the appropriate time to signal readiness.145-146: The addition of autocmds for
User DenopsReadyandUser DenopsClosedis consistent with the PR's goal to manage server readiness. Ensure that these autocmds are triggered at the correct moments and that their handlers (s:DenopsReadyand settings:is_readyto false) are appropriate.167-168: The configuration parameters
denops#server#wait_intervalanddenops#server#wait_timeoutare introduced. Confirm that these values are sensible defaults and that they are used correctly wherever waiting logic is implemented.
lambdalisue
force-pushed
the
fix-denops-ready
branch
from
37bfbfd to
ee99de0
Compare
lambdalisue
changed the title
DenopsReady and initialization of meta
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (12)
- autoload/denops.vim (1 hunks)
- autoload/denops/_internal/server/chan.vim (1 hunks)
- autoload/denops/plugin.vim (4 hunks)
- autoload/denops/server.vim (5 hunks)
- denops/@denops-private/cli.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (3 hunks)
- denops/@denops-private/host/vim.ts (4 hunks)
- denops/@denops-private/service.ts (6 hunks)
- denops/@denops-private/worker/script.ts (4 hunks)
- doc/denops.txt (6 hunks)
- plugin/denops.vim (2 hunks)
Files skipped from review as they are similar to previous changes (10)
- autoload/denops.vim
- autoload/denops/_internal/server/chan.vim
- autoload/denops/plugin.vim
- autoload/denops/server.vim
- denops/@denops-private/cli.ts
- denops/@denops-private/host/invoker.ts
- denops/@denops-private/host/vim.ts
- denops/@denops-private/service.ts
- denops/@denops-private/worker/script.ts
- plugin/denops.vim
Additional comments: 10
denops/@denops-private/host/nvim.ts (4)
28-28: The addition of the
#invokerfield to theNeovimclass is a significant change. Ensure that theInvokerinstance is always set before any invocation occurs to avoid runtime errors.37-45: The
voidmethod is added to the dispatcher to handle void messages. This is a good practice as it explicitly handles cases where no return value is expected.46-53: The
invokemethod is updated to use the#invokerfield. It is crucial to ensure thatthis.#invokeris notundefinedwhen this method is called, as it throws an error otherwise. This change centralizes the invoker logic, which is a good practice for maintainability.112-112: The
registermethod now assigns theinvokerto#invoker. This change is part of the refactoring to use the#invokerfield within the class. It's important to ensure thatregisteris called before any invocation methods to prevent runtime errors.doc/denops.txt (6)
93-93: The documentation now specifies that
g:denops#debugmust be configured before denops initialization. This is an important update that aligns with the code changes and should be clearly communicated to users.133-139: New variables
g:denops#server#wait_intervalandg:denops#server#wait_timeoutare introduced with default values. This addition provides users with more control over the server wait behavior and should be clearly documented.229-231: The function
denops#server#stop()has been renamed todenops#server#close(). This renaming should be reflected in all parts of the documentation and the codebase to maintain consistency.250-265: The addition of
denops#server#wait()anddenops#server#wait_async()functions in the documentation is crucial for users to understand the new synchronous and asynchronous waiting capabilities for theDenopsReadyautocmd.305-324: The documentation marks
denops#plugin#register()as deprecated and recommends usingdenops#plugin#load()instead. It also provides a fallback example for backward compatibility. This is a good practice to ensure smooth transition for users.327-332: The
denops#plugin#discover()function's behavior change is documented. It now loads plugins instead of registering them, which is a significant change that plugin developers need to be aware of.
|
It seems tests are broken. I'll fix later |
lambdalisue
force-pushed
the
fix-denops-ready
branch
from
ee99de0 to
4889eae
Compare
|
Fixed |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (12)
- autoload/denops.vim (1 hunks)
- autoload/denops/_internal/server/chan.vim (1 hunks)
- autoload/denops/plugin.vim (4 hunks)
- autoload/denops/server.vim (5 hunks)
- denops/@denops-private/cli.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (3 hunks)
- denops/@denops-private/host/vim.ts (4 hunks)
- denops/@denops-private/service.ts (6 hunks)
- denops/@denops-private/worker/script.ts (4 hunks)
- doc/denops.txt (6 hunks)
- plugin/denops.vim (2 hunks)
Files skipped from review as they are similar to previous changes (11)
- autoload/denops.vim
- autoload/denops/_internal/server/chan.vim
- autoload/denops/plugin.vim
- autoload/denops/server.vim
- denops/@denops-private/cli.ts
- denops/@denops-private/host/invoker.ts
- denops/@denops-private/host/vim.ts
- denops/@denops-private/service.ts
- denops/@denops-private/worker/script.ts
- doc/denops.txt
- plugin/denops.vim
Additional comments: 4
denops/@denops-private/host/nvim.ts (4)
28-28: The addition of the
#invokerfield is consistent with the PR's objective to improve the plugin's initialization process. This field will store theInvokerinstance.37-55: The dispatcher's
invokemethod is now an async function that asserts the method and arguments, and if the#invokeris set, it calls the corresponding method on the#invoker. This change centralizes the invocation logic and error handling.112-112: The
registermethod is updated to assign theinvokerto the#invokerfield. This is a straightforward change that supports the new invocation logic.42-43: The
voidmethod in the dispatcher is a stub that resolves immediately. This seems to be a placeholder for future functionality or a method to satisfy an interface contract.
|
The conflict exists. |
Previously, the `DenopsReady` autocmd was fired when the channel was connected. However, it is not the right timing because the denops itself may not be ready yet. This commit fixes the timing of the autocmd so that denops is ready when the autocmd is fired.
This is a minor but a breaking change. This change affects the `g:denops#debug` variable. After this commit, the variable must be configured prior to the denops initialization.
It seems `timer_start()` triks is no longer needed so we can call callbacks synchronously in both cases.
Now developers can wait until the denops is ready to use.
The trace feature is not available when denops liberates from Worker and the feature obstructs the development of denops.
We are planning to change internal registration mechanism and the autocmd is not suited for the new mechanism. Quick search on GitHub shows that the autocmd is not used by anyones. https://github.com/search?q=DenopsPluginRegister&type=code
We found that developers always use `mode: "skip"` and other options are
not necessary. So we decided to remove `{options}` from functions and
always use `mode: "skip"`.
Quick search revelaed that no one uses `reload` and `discover` with
`{options}` but some (e.g. dein.vim) uses `register` with `{options}`.
That's why we introduced a new `load` function that replaces `register`.
Developers who need to support previous denops should use `load` whenever
possible like:
```vim
try
call denops#plugin#load(
\ "denops-hello",
\ "/path/to/denops-hello/main.ts",
\)
catch /^Vim\%((\a\+)\)\=:E117:/
" Fallback to `register` for backward compatibility
call denops#plugin#register(
\ "denops-hello",
\ "/path/to/denops-hello/main.ts",
\ {"mode": "skip"},
\)
endtry
```
lambdalisue
force-pushed
the
fix-denops-ready
branch
from
4889eae to
be0bd9b
Compare
|
I will test it. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (12)
- autoload/denops.vim (1 hunks)
- autoload/denops/_internal/server/chan.vim (1 hunks)
- autoload/denops/plugin.vim (4 hunks)
- autoload/denops/server.vim (5 hunks)
- denops/@denops-private/cli.ts (2 hunks)
- denops/@denops-private/host/invoker.ts (2 hunks)
- denops/@denops-private/host/nvim.ts (3 hunks)
- denops/@denops-private/host/vim.ts (4 hunks)
- denops/@denops-private/service.ts (6 hunks)
- denops/@denops-private/worker/script.ts (4 hunks)
- doc/denops.txt (6 hunks)
- plugin/denops.vim (2 hunks)
Files skipped from review as they are similar to previous changes (11)
- autoload/denops.vim
- autoload/denops/_internal/server/chan.vim
- autoload/denops/plugin.vim
- autoload/denops/server.vim
- denops/@denops-private/cli.ts
- denops/@denops-private/host/invoker.ts
- denops/@denops-private/host/vim.ts
- denops/@denops-private/service.ts
- denops/@denops-private/worker/script.ts
- doc/denops.txt
- plugin/denops.vim
Additional comments: 3
denops/@denops-private/host/nvim.ts (3)
- 28-28: Ensure that
#invokeris initialized before any method that relies on it is called to prevent potential runtime errors.Verification successful
The search results indicate that
#invokeris used in several places withinvim.tsandnvim.ts. Notably, innvim.ts, there is a check forthis.#invoker === undefined, which suggests that there is a guard to ensure#invokeris notundefinedbefore it is used. This addresses the concern raised in the review comment about ensuring that#invokeris initialized before any method that relies on it is called.Based on the provided output, the following can be concluded:
#invokeris assigned innvim.tsandvim.ts(lines 77 and 112 respectively).- There is a check for
undefinedinnvim.ts(line 47) before#invokeris used.#invokeris used to dispatch a method invim.ts(line 36) and to invoke a method innvim.ts(line 53).Given this information, it appears that the code has been structured to prevent runtime errors by checking
#invokerforundefinedbefore use.* 46-53: The addition of the `invoke` method with a check for `#invoker` being undefined is a good practice to ensure that the `Invoker` is registered before invocation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of `#invoker` to ensure it's always checked for undefined before use. ast-grep --lang typescript --pattern $'this.#invoker.$_($_)'Length of output: 61
Script:
#!/bin/bash # Search for any usage of `#invoker` to see if it's being used in any other context or if there are checks for `undefined`. ast-grep --lang typescript --pattern $'this.#invoker'Length of output: 455
- 112-112: The
registermethod correctly assigns theinvokerto#invoker, providing a clear initialization point for theInvoker.
🐛 Fix fire timing of DenopsReady autocmd
Previously, the
DenopsReadyautocmd was fired when the channel was connected. However, it is not the right timing because the denops itself may not be ready yet. This commit fixes the timing of the autocmd so that denops is ready when the autocmd is fired.If this change cause issue, developers should use
denops#server#wait()ordenops#server#wait_async()function to ensure that functions are called AFTER denops got ready. For example💥 Read meta only once in initialization
This is a minor but a breaking change. This change affects the
g:denops#debugvariable. After this commit, the variable must be configured prior to the denops initialization.💥 Synchronously call wait_async() callbacks
It seems
timer_start()triks is no longer needed so we can call callbacks synchronously in both cases.Let me know if this cause acutal issues (it might break lazy loading mech of denops that are not tested by myself.)
💥 Remove g:denops#trace option
The trace feature is not available when denops liberates from Worker and the feature obstructs the development of denops.
💥 Remove DenopsPluginRegister:* autocmd
We are planning to change internal registration mechanism and the autocmd is not suited for the new mechanism.
Quick search on GitHub shows that the autocmd is not used by anyones.
https://github.com/search?q=DenopsPluginRegister&type=code
💥 Remove {options} from register, reload, discover
We found that developers always use
mode: "skip"and other options are not necessary. So we decided to remove{options}from functions and always usemode: "skip".Quick search revelaed that no one uses
reloadanddiscoverwith{options}but some (e.g. dein.vim) usesregisterwith{options}.That's why we introduced a new
loadfunction that replacesregister.Developers who need to support previous denops should use
loadwhenever possible like:Summary by CodeRabbit
Summary by CodeRabbit
Refactor
New Features
Documentation
Bug Fixes