-
Notifications
You must be signed in to change notification settings - Fork 26
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
Create POST-MORTEM document #249
Merged
Merged
Changes from all commits
Commits
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 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,100 @@ | ||
# Purpose | ||
|
||
This document will try to gather all the valuable information, debugging processes and insights developers could get | ||
during the process of Playground improvement. | ||
|
||
# [Major LSP Refactoring - PR #238](https://github.com/onflow/flow-playground/pull/238) | ||
|
||
## Problem | ||
|
||
The work encapsulated in that PR was targeted at the problem of state de-sync: contract could be updated/redeployed, but | ||
script importing said contract would not be aware of newly created fields or methods. As a result, user would need to | ||
refresh webpage to be able to use updated contract code. | ||
|
||
Said problem existed for quite some time in Playground and was not properly addressed because of lack of resources and | ||
priorities in other Developer Tools. | ||
|
||
## Hypothesis | ||
|
||
One of the components is caching or translates outdated code into LSP (_Language Server Protocol_). | ||
|
||
## Development Challenges | ||
|
||
- LSP Client and Server are nested deeply into React tree, which leads to unnecessary code updated and server/client | ||
restarting on each state change. | ||
- Existing Webpack, eslint and Typescript compiling would create slow pipeline, resulting in 40 seconds update even on a | ||
minuscule change | ||
- No `hot-reload`, meaning you will lose some changes you've done to the application state and slowing down testing | ||
process | ||
- No automatic test pipeline | ||
|
||
## Approach | ||
|
||
### Make Typescript Less Strict | ||
|
||
Until the source of the problem is not identified we disabled Typescript strict checks to speed up linter (even for a | ||
bit) and compiler. This allowed us to focus on solution and not fighting the type system. | ||
|
||
### LSP Components as React Context | ||
|
||
LSP components were moved into separate React context provider (right below Project provider, because we need that | ||
dependency to pass code to server) to allow any components located below in the tree to access the values and methods | ||
without `props drilling` and unnecessary updates. | ||
|
||
### Decouple LSP components and Monaco Editor | ||
|
||
Monaco Editor is structured in such a way that you don't need to directly connect it to LSP components. Previous | ||
implementation required language server dependency so that "control panel" could get a list of arguments by sending a | ||
specific message. | ||
|
||
## Findings | ||
|
||
### Callback Closures | ||
|
||
#### Problem | ||
|
||
Setting up callbacks on the language client would create a closure around current props/state of the components. All the | ||
following methods used within said callbacks will use old values. | ||
|
||
#### Solution | ||
|
||
Language Client callback setters will return a `Disposable` object. With help of `useRef` and `useEffect` React hooks we | ||
keep them updated every time important dependency changes - usually it's either `project.accounts` or `languageServer` | ||
value. | ||
|
||
### Caching | ||
|
||
#### Problem | ||
|
||
Quick search through [Cadence repository](https://github.com/onflow/cadence) was fruitless to uncover any caching | ||
mechanism for caching the code during import resolvers, so we dismissed the idea that something is wrong with Language | ||
Server and focused on the webapp itself. But after sprinkling `getAddressCode` method - which language server is setting | ||
during initialization - with a bit of `console.log` calls, we've spotted that Language Server never fetches code twice | ||
for the same contract from account. Another engineer decided to scratch that one for a bit, while we were patching other | ||
"holes" in webapp. In the end he found that there is indeed a caching mechanism, which was preventing re-fetching of | ||
contract code. | ||
|
||
Incorporating fix for the caching would allow us to instantiate LSP components single time, when the app starts and | ||
eliminate the need to restart language server every time the contract code changes. This will be done in following PRs, | ||
when new package for Cadence language server is released. | ||
|
||
#### Solution | ||
|
||
We identified that out of two LSP components - client and server - it's server that responsible for caching and updates. | ||
Using `useEffect` React hook we restart language server every time account's contract code is changed (value change is | ||
actually debounced, so we don't need to it as user types, but only when it's saved). | ||
|
||
### Old Editor State | ||
|
||
#### Problem | ||
|
||
Let's say you use some non-existing method in your transaction/script. Editor would highlight you the issue - show | ||
`diagnostics` - using LSP components. Then you switch to contract code, edit something and redeploy. Language server | ||
restarted, but as you switch back to the script you still see the error. That's because editor does not know anything | ||
about changes in the imported code and "thinks" everything is the same. | ||
|
||
#### Solution | ||
|
||
We've used the same trick as previously implemented in VS Code extension - add new line character at the eof, them add | ||
timeout and revert code to original state. This will "trick" editor into thinking that code was changed and need to be | ||
checked for errors again, catching new changes on contract in the process. Win-win! :) | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaxStalker if my understanding is correct -- the problem was when a user switched back to script while language server is not fully done with processing the changes made, it would result in error.
But is the solution mentioned here the appropriate fix? It sounds like we are adding an unnecessary EOF just for the sake of triggering some actions. could there be a cleaner way to handle this? I'm not familiar with the code base but by thinking aloud, is this possible: do not allow transaction/script/contract view switch until language server is fully restarted and having the latest information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really about the latest state of the language server, but rather about the way to ask language server to run the check again. I couldn't find a way to trigger re-check manually and not sure it actually exposed on Monaco editor 🤷♂️
And we don't add any footprint to the file, as we remove new line character after a small delay, which in turn triggers recheck.