Skip to content
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

WIP: Add webview output experience #2481

Draft
wants to merge 39 commits into
base: dev
Choose a base branch
from
Draft

Conversation

bpringe
Copy link
Member

@bpringe bpringe commented Mar 31, 2024

What has changed?

A new webview for REPL output is added. It will be a new output location in settings. This is still in the exploratory phase (sort of). I'm opening this draft PR now for visibility and also so I can use it quickly gain context again when I pick this work up here and there as I have time. I'm exploring unknowns and potential problem areas before spending much time on stuff we know we can do.

Once this is in a usable state I plan to dogfood is by using it for work for a while before releasing it.

This is also an experiment in adding a new feature almost completely in cljs, which utilizes the VS Code API.

Closes #2480

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (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 in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • 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.
  • 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.
  • Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
  • Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik

Copy link

netlify bot commented Mar 31, 2024

Deploy Preview for calva-docs ready!

Name Link
🔨 Latest commit 32c27e7
🔍 Latest deploy log https://app.netlify.com/sites/calva-docs/deploys/664a820d4cfe09000798c780
😎 Deploy Preview https://deploy-preview-2481--calva-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

package.json Outdated
@@ -3254,6 +3254,7 @@
"open": "^6.3.0",
"parinfer": "^3.12.0",
"posthtml-parser": "^0.11.0",
"react-dom": "^18.2.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we need React for something in particular. If we do, then of course we should use it. Otherwise, it may be better to use something with less bloat, like Dumdom, or maybe Replicant.

Copy link
Member Author

@bpringe bpringe Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for reagent, which I consider very lightweight and should be used lightly for this. If you think there's any performance issue or other issues with it that would be significant for our use-case, we should discuss that, but switching it out later should be pretty simple.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to alternatives if there's a good reason to use something else.

Copy link
Member Author

@bpringe bpringe Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumdom looks interesting. Using a library that supports react gives us reach into the react ecosystem though, which could be nice in the future for more rich features and interactivity (making it less likely we'll need to recreate the wheel).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My experience with React is that it changes all the time in breaking ways, making it high maintenance. With Dumdom we would avoid this, as the vdom library it uses is super tiny and stable. With Replicant we would get a pure ClojureScript vdom as well, and have even more of the familiar Clojure stability (though I am not sure if the Replicant API is marked as stable yet).

However, I am not opposed to React if we have a good reason to use it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say we move forward with it for a first-release, and maybe be conscious of our usage of it (keeping it light), and if we come across issues we can switch it out later.

I'll continue to keep other options in mind and think about the React usage for now...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say we move forward with it for a first-release

I may reconsider this. I need to look at Dumdom more closely.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having had a closer look at Replicant, In think it's better to use that instead of Dumdom. They are quite similar, but Replicant drops the dependency on Snabbdom, as well as some legacy API requirements that Dumdom carries.

@PEZ
Copy link
Collaborator

PEZ commented Apr 6, 2024

Shall we try to release this behind an opt-in setting as quickly as we can?

@bpringe
Copy link
Member Author

bpringe commented Apr 13, 2024

Shall we try to release this behind an opt-in setting as quickly as we can?

Sorry, I'm moving slowly with this so far due to being busy with other life things. It's not quite ready but there's not that much more to do before getting it to a very basic usable state. I've started dedicating a little focused dev time on weekends for this.

@bpringe bpringe marked this pull request as draft April 23, 2024 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants