add initial idea for flares
DRAFT
What has changed?
- Evaluate now calls flareHandler.inspect on all values it receives from the REPL server
- Added flareHandler that identifies flares (requests for IDE behavior) and dispatches them by type
- Added a webview constructor to handle flares requesting to display html
Fixes #2679
My Calva PR Checklist
I have:
- [*] Read How to Contribute.
- [*] Directed this pull request at the
devbranch. (Or have specific reasons to target some other branch.) - [*] Made sure I have changed the PR base branch, so that it is not
published. (Sorry for the nagging.) - [*] Made sure there is an issue registered with a clear problem statement that this PR addresses, (created the issue if it was not present).
- [*] Updated the
[Unreleased]entry inCHANGELOG.md, linking the issue(s) that the PR is addressing.
- [*] Updated the
- [ ] Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
- [x] Added to or updated docs in this branch, if appropriate
- [ ] Tests
- [ ] Tested the particular change
- [ ] Figured if the change might have some side effects and tested those as well.
- [x] Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run
npm run prettier-format) - [x] Confirmed that there are no linter warnings or errors (use the eslint extension, run
npm run eslintbefore creating your PR, or runnpm run eslint-watchto eslint as you go).
Ping @pez, @bpringe, @corasaurus-hex, @Cyrik
Deploy Preview for calva-docs ready!
| Name | Link |
|---|---|
| Latest commit | 65d6fb02ca80a767c033f2f3e9389a11208bc891 |
| Latest deploy log | https://app.netlify.com/sites/calva-docs/deploys/67db34e99fec660008064e69 |
| Deploy Preview | https://deploy-preview-2680--calva-docs.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Hi @PEZ.
I think this proposal by @timothypratley is a great idea that will be very helpful in our plans to make Clojure beginner-friendly.
The Flares approach will allow us to add functionality that will be seamless to a new user (e.g., compared to installing Joyride and adding a Joyride script). The fact that we can ship it as part of our JVM Clojure tools and libraries matters a lot.
Is there anything we can do to help promote this?
I thought there was an open question how to implement the flares. Go the same way that @cursive-ide takes it, or some other way. I have to admit I have not fully understood the trade-offs and implications, but if we can share things easier with Cursive by implementing things the same way, than that would be a win.
Summary
There are 2 proposals for displaying WebViews in Calva; both work well
- This PR detects a special value and triggers IDE behavior (WebView) similar to Cursive. There is a small difference that instead of using tagged literal form
#cursive/html{...}it uses a normal map{:calva/flare {:type :wevbview ...}} - And another one #2684 that only triggers IDE behavior when the code was invoked from a custom REPL command snippet, and allows any IDE command. It is a flag on the snippet configuration. Because it invokes commands it uses
[cmd args....]to represent the request.
I don't think that Colin will change the Cursive implementation, so the only question is what is best for Calva?
Tradeoffs
Safety: Option 1 applies to all data evaluated in a REPL, it could be malicious data from a database or log file. Therefore the capability should be strictly limited. Is showing HTML and external pages from a URL acceptable? Option 2 applies only to code which brings no additional risk (we already trust libraries) so can possibly do more than WebViews and info.
Access: Option 1 allows users and libraries to make use of flares without configuring or exporting snippets.
Functionality: Do we want WebViews? (yes!) Do we want to be able to show Info messages in the IDE? (I think so, but we could leave it out for now) Do we want to be able to run other IDE commands? (Maybe, but there doesn't seem to be a compelling use for it)
IDE Request syntax: Should it be a tagged literal? a map? a vector? I don't think it matters much, but one thing to notice is that in options (2) snippets are configured to expect ONLY a command as the result, so they have a less unique shape where as (1) they can occur anywhere so they need to be more unique. Possibly we can follow Cursive more closely with tagged literals.
Recommendation
Proceed with option 1, but remove the "command" flare for safety
Ask
Yes, we need a decision from you @PEZ on how to move forward here. It's quite a complicated tradeoff question for a very basic feature, but it will be so valuable to be able to show visualizations. I'm available any time to discuss, show or do interactive code review if that helps.
Thanks for clarifying, @PEZ @timothypratley. This helps a lot.
I think the discussion together with Cursive was worth the try (and insightful to read), but since we could not find a way to make that discussion proceed, it makes sense to keep going here and think what is best from the Calva perspective.
~~I like Timothy's recommendation to proceed with option 1. It will allow updating the WebView from within the user program, not only through custom commands. This allows, for example, to open and update the WebView as a response to events such as a file change. This can be relevant to live-reload experiences of various tools (Figwheel, Shadow, Oz, Clerk, Clay).~~
~~We discussed such a behaviour in a couple of recent videos.~~ ~~* Noj Reload Executable~~ ~~* Noj in a JAR~~
Edit: removed my last comments which were misleading (actually unrelated to this feature).
I'm sorry, I didn't realize that I have been stalling this.
Since I am not very familiar with the usecase I think combining the judgement of you two: @timothypratley and @daslu here is best for Calva users. Option 1 sounds good to me. I have some questions/comments:
- Evaluating code already can wipe your hard disk or whatever. How would allowing IDE commands bring any extra danger to it?
- Does it make sense to name it like
{:flare/html ...}rather than{:calva/webview ...}? This way any repl client is invited to implement it, rather than pointing it at Calva and how it should be implemented. - I like the suggestion to go with tagged literals.
Was this what you need from me, @timothypratley? Just let me know, I am a big fan of this whole idea, and would hate to be stalling it further. π
Yes this is great feedback. Thank you.
- The extra danger is that enabling commands would enable data to wipe your hard disk. We accept the code risk, but data risk is something new. We have a level of trust in code because it comes from verified sources, but data does not have any trust protection. The extra risk is similar to SQL injection attacks; a users enters data like my "username" is "DROP TABLES" and the result is that a database is destroyed. The only way to protect against it is to treat user input carefully when creating sql queries. In the REPL we produce values, possibly from untrusted sources, sometimes without realizing it. We don't need to execute external code to create a flare, we just read it in data we are looking at. If there is an exploit, bad actors have a motivation to sprinkle bad data around where you will find it, when entering a "username", or in edn files, or in log messages. In my opinion we should limit the capabilities of flares to specific features like webview that are not expected to be harmful.
2/3. Yes; all of the options you mentioned are great, which one should I change it to?
I think the next steps are:
- I should remove the "command" flare from this PR if you agree that it doesn't add value for the risk
- I should change the shape of flares to
Then it should be ready for a final code review by you and hopefully release π
@PEZ @daslu what do you think the flare syntax should be?
Cool. Let's remove the command flare for now at least.
Syntax... So something like:
#flare/html {:title ... :html ... ...}
#flare/message {:type <info|warning|error> :message ... ... }
WDYT?
Makes sense to me!
@PEZ @daslu O.K. I've updated the code and documentation to the tagged literals you recommended, and removed commands, so it's over to you now π To try it out, there are some examples in the docs. Please let me know if further changes are required.
Awesome. The code looks easy to understand, extend and maintain.
You think you can create an example project that we can use from the documentation? It would make it easier to grasp the concept if the user can go through some quick steps that are already prepared.
@PEZ what do you mean by an "example project"?
The examples in the documentation: https://deploy-preview-2680--calva-docs.netlify.app/flares/ don't need any setup, you just past them into your REPL or code and evaluate them. Maybe the docs can be improved if it isn't clear what they do and why it is useful?
Did you have an idea for an "example project"? (I'm happy to provide one if I can understand a little better what you mean)
I mean something like a repo on Github that you can clone and evaluate some stuff in and have these cool things happen. It was the Clay mention in the docs that started me to wish there was an example project that I could try. It'll get more real with some graphs and stuff showing up. Usually we wouldn't actually need to hold off merging this because of this, but it would help me review and test how things work.
Checking the docs again now, I agree with your observation that it could be clearer with that some of the examples can be just copied and evaluated with Calva's repl connection. Maybe a section like βtry these in your replβ? Could be a rich comment form. And we could also have some maven coordinates to use for getting Clay into the mix.
@PEZ @daslu I've updated the docs accordingly:
- Simplified and clarified that the examples can be tried at the REPL
- Added a more compelling example that makes an SVG image, and supplied a screenshot
- Gave more context about Clay and linked to Clay visualization examples
- Emphasized the
flare/htmland de-emphasized theflare/message, as for now the main exciting feature is the ability to show HTML.
I didn't create a repo because the examples are small enough to copy+paste.
For a more spectacular demo, Noj has many notebooks which can be used to show interesting visualizations, and Clay has an examples page that I linked to in the documentation.
Please let me know if you have something else in mind.
To explain my confusion about the repo idea: Clay and Noj are all about creating notebooks, so there are already many great examples to choose from if we want to show charts. Maybe @daslu can recommend one in particular, or if you like we can work on something new together if we have a clear idea of what it should contain.
The updated documentation can be viewed here: https://deploy-preview-2680--calva-docs.netlify.app/flares/
Thanks, I hope to try it today.
Here is one repo for example, that has a meaningful data-analysis example. https://github.com/scicloj/noj-v2-getting-started
It may be convenient to explore, because it has a recorded video demonstrating the current experience we have in Clay.
If that seems helpful, I'll try creating a version of Clay that uses the flares.
I love the updated docs, @timothypratley. β€οΈ
Some doc comments:
- We could mention that it works in any type of REPL: Clojure, ClojureScript, Babashka, Joyride. It is sort of given, but may help people to quicker grasp what is going on.
- Now that we don't have a Clay example in the docs I think we can do without an example project for that. But I do think this will be a good add later. I'll have a look at the project you showed us at, @daslu, and see if I can describe what an example project could be about after having gained some more insight.
- It would be great with one or two examples how to use
#flare/message - The docs for
#flare/messagelack the:messageattribute
Quirks/bugs
support for MessageItem objects
This one may be between docs or implementation: I tried to us a vscode.MessageItem object for the :items and it gives me an error:
(tagged-literal 'flare/message {:type :info
:title "Calva homepage"
:message "Hello world"
:items [{:title "OK"
:isCloseAffordance false}
{:title "Cancel"
:isCloseAffordance true}]
:then 'clojure.core/println})
=> ((resolve 'clojure.core/println) {"title":"Cancel","isCloseAffordance":true})
:true
[line 1, col 43] Invalid keyword: :.
------ WARNING - :undeclared-var -----------------------------------------------
Resource: <eval>:1:1
Use of undeclared Var user/Cancel
--------------------------------------------------------------------------------
[line 1, col 2] Unmatched delimiter ).
It renders the buttons as I expect (Cancel being a close/dismiss button).
Here's a similar invokation using Joyride:
(-> (vscode/window.showInformationMessage
"Hello"
#js {:title "OK", :isCloseAffordance false}
#js {:title "Cancel", :isCloseAffordance true, :payload (clj->js {:foo [1 2 3]})})
(.then (fn [x] (def x x) (println x))))
#<js/Promise[~]>
#js {:title Cancel, :isCloseAffordance true, :payload #js {:foo #js [1 2 3]}}
The .then function gets the full message item, including any arbitrary payload (:payload isn't part of the MessageItem shape, just something I chose).
If it's not trivial to fix now, we can lock it down to strings and provide an error message if the user tries to use something else. And add MessageItem support later.
I note that the QuickPick API is similar, and see #flare/menu as something we can also add later.
Untitled files/tabs
A thing I often do to just try something out at the REPL is to open an untitled file/tab (cmd+n) and set it to be a Clojure file (cmd+k, m). When I tried this with a flare I got an error. But then I couldn't reproduce it, so I really don't know what happened. I don't think we should spend time on chasing this ghost, but something to be aware of, maybe.
For message items, on the other branch I have some code that attempts to map Clojure data to vscode objects, which mostly worked π
Quickpick sounds interesting.
Other possibilities for flares:
"workbench.action.quickOpen" "app.clj" "simpleBrowser.api.open" "https://calva.io/" "vscode.diff" ["filea.txt" "fileb.txt" "diff title"
Messages etc all seem to be "promising" but with no concrete use right now.
Maybe I should just remove them for now, and we can always add them later if someone comes up with a use case?
Yeah, we could also wait for use cases before adding.
π O.K. I removed the message flare for now.
@PEZ let me know if I missed anything π
Meanwhile it looks like the current dev branch has a bug so I can't start a REPL anymore... it looks like it should be easy to fix but I don't want to cause merge conflicts, so I won't do anything about it unless you say to.
Awesome. Thanks a ton for adding this to Calva, @timothypratley ! β€οΈ
I think the error you see is also in the released version of Calva. Do you have a custom connect sequence with auto-connect or auto-jackin enabled?
Yes, this is my settings.json where the problem occurs:
{
"calva.autoStartRepl": true,
"calva.replConnectSequences": [
{
"name": "Workspace",
"projectType": "deps.edn",
"autoSelectForJackIn": true,
"menuSelections": {
"cljAliases": ["dev", "test"]
}
}
]
}
I mistakingly made projectRootPath mandatory in the previous release of Calva. It will probably stay that way, but I should fix the error message at least...
Oh I see, adding "projectRootPath": ".", to settings fixed it. π