State issue with released cljs/js interop

[bpringe]

This issue didn't pop up in dev, and unfortunately I didn't catch it before I released.

From what I can tell, the issue is related to how we output the code for release, and it revolves around the state module, though it could pop up elsewhere. In order for the js/require calls to work, the individual .js files for those modules need to exist, so I added a step to the release script to compile with tsc before bundling with webpack. However, it seems this causes an issue in that two different modules can be used for the same intended module.

I believe the js code uses the bundled state module, but the cljs code uses the unbundled state.js file, so when js updates a state value, cljs can't see the update, and vice versa. This leads to issues such as the cljs not being able to see if the repl is connected (because that's updated from js) and the js not being able to get the lsp client from state (because that was set in cljs). Put simply, there's two versions of state, one for js and one for cljs.

I think there's a way to solve this, besides writing the state module in cljs, which is an option, but it'll require some thought.

[bpringe]

Thinking the simple answer lies in how we configure webpack, or maybe not using webpack.

[PEZ]

Ah, yes. I think I have stumbled across this as well, but forgotten about this particular problem.

We'll figure it out!

[bpringe]

I've explored different paths for this now, with one remaining to be explored. The issue lies in the fact that we're bundling with two different tools, shadow for cljs, and webpack for ts. It seems we cannot bundle in this way and make both cljs and ts depend on each other. We can make the ts depend on the cljs as we've been doing, but not also the other way around.

The main issue when trying to bundle is with the state module. The cljs code and js code write to and read from different states. They can't read a state value the other one writes, which is problematic.

Here's what seems to be our remaining options:

• We can make both sides of the code depend on each other if we don't bundle at all. This increases the Calva vsix size a bit. This also increase initial install time by a bit, still only a few seconds or so, but I see not much difference in load time. This would need to be tried for a while to really see if it makes a difference.
• Make shadow-cljs bundle everything. I haven't tried this but I'm wondering if we can use an extension.cljs that requires extension.js, and calls its activate and deactivate functions, serving as a shell and entry point for shadow-cljs. As I think Thomas Heller mentioned, we may need to use simple optimization to avoid some issues with the js bundling (advanced could mangle things in a way that could break things?).
• If the above two are not suitable / don't work, we can try just rewriting the state module in cljs. This might make everything else just work as we intend.

[bpringe]

When working toward the "bundle all with shadow" path, when I try to require the config.js file like ["/config.js" :as config] with the out directory on the classpath, the code is compiled without error by shadow, but when I try to access something in config like (.. config2 -default -REPL_FILE_EXT) (something that works when pulling it in with js/require), I get an error: module$config is not defined.

I think this has something to do with the way the .js file is written by tsc, but I've tried changing it with no luck (still get the error even if I use module.exports instead of exports.default. This issue seems related, but like I just mentioned, I had no luck following what seemed to work there (though maybe I still missed something - looks they they weren't using the :node-library target). I'm also using a later version of shadow than the mentioned version that contained a fix.

The compiled config.js that does not work is:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const config = {
    REPL_FILE_EXT: 'calva-repl',
    KEYBINDINGS_ENABLED_CONFIG_KEY: 'calva.keybindingsEnabled',
    KEYBINDINGS_ENABLED_CONTEXT_KEY: 'calva:keybindingsEnabled'
};
exports.default = config;
//# sourceMappingURL=config.js.map

I may leave this method alone for a bit and explore another route.

[bpringe]

@PEZ Is there a reason for not using the :npm-module shadow target? I ask because I started to explore using that to get the bundling-all-with-shadow working, but likely don't need to go that route anyway.

On another note, I found that the current issue with two different states being used when using webpack to bundle can be solved by creating a simple state api in cljs like:

(ns calva.state)

(defonce ^:private state (atom {}))

(defn set-state-value! [key value]
  (swap! state assoc key value))

(defn get-state-value [key]
  (get @state key))

(defn get-state []
  @state)

I verified that js sees state set by cljs and vice versa with this (added exports in the shadow build for those functions). This also lets us move only the state storing part of the state TS module out, so we don't have to rewrite the rest, though maybe a rename or extracting parts out to other modules would be a good idea to avoid confusion.

Another neat thing about cljs for state is we could use clojure.spec to validate state, if we wanted. I don't know that it would add much, but this comment made me think about that:

calva/src/state.ts

Line 100 in c8543d7

// TODO find a way to validate the configs

Speaking of that comment, we could move that config stuff into the cljs state ns, but I was thinking maybe a config ns would be better, but we could use clojure.spec there too to validate config.

Another neat thing is that we could easily set state from the repl in development to more quickly test scenarios involving state, if we wanted.

[PEZ]

There was a reason for not building an :npm-module, even if I have forgotten what it was now. On a high level it was that it didn't work. Thomas Heller and I pursued it quite a lot, but eventually it was only :node-library that was left as an option. But I don't think :node-library scales very well, the API gets very ”wide” at the edge. Maybe things have changed with the :npm-module target, so it could be worth investigating again.

I don't recall right now many places where we use the state from the state module, but it could be that most (all?) can be moved to vscode state. Not saying we shouldn't just use an atom. Just throwing the vscode path in as an option, in case you haven't considered. We should be able to REPL that state too, right?

In any case I think it would be a good idea to split apart the state module in modules with more clear and focused concerns. Then it should get easier to move things into cljs in smaller steps.

Since I wrote that TODO comment, vscode's JSON editor has improved a lot. It's quite convenient for the user to get the config right, these days. At least as far as ”schema adherence” is concerned. Not sure there is very much room for improvement there. (But again, I might have written that comment b/c reasons, even if those escape me right now...)

I get this feeling we should have kept a decision log. Even if that might be a bit tricky, of course.

[bpringe]

But I don't think :node-library scales very well, the API gets very ”wide” at the edge.

Yeah, this is basically why I was exploring :npm-module, but at the same time, I'm not sure how much it will grow if things were to be converted to cljs over time, some new things written in it, etc., because those things can use the cljs directly and less js will need to depend on cljs. Might be something to revisit later if it seems to be an issue.

:node-library does seem to make sense if the goal is to have more written in cljs and bundled by shadow. I think bundling of :npm-module output still needs to be done by a js bundler like webpack, and the entry point is meant to be in js with that target, I think (could be wrong there). Thinking that because of the description of the target.

Anyway, I'm looking into vscode state as I hadn't before. Will ponder the implications of using that vs an atom.

About the decision log, I sort of use issues for that, even if it looks like I'm talking to myself 😄. Probably not the best method, though.

[bpringe]

Having considered both, I think at least for global ephemeral state, it makes sense to use an atom (so, in places where we use Immutable currently). For persistent state, it makes sense to keep using the workspaceState and globalState of the vscode context.

I was starting to ponder adding watchers to the state atom to use a reactive style of programming, but then it seemed like I was going down a path toward re-frame 😄. But, I wonder how it would be to use re-frame for Calva 🤔. Just thinking here, not about to do anything crazy right now.

[bpringe]

As far as I can tell this is solved by #1053. I think as long as we aren't storing state in different places, we shouldn't have this problem. We'll see if this crops up elsewhere in the future.