(require) does not load ns from user script dir

[bobisageek]

OS: Windows 10
Joyride version: 0.0.7

Repro:
$HOME/.config/joyride/scripts/user_lib.cljs

(ns user-lib
  (:require ["vscode" :as v]))

(defn info-message [msg] 
  (v/window.showInformationMessage msg))

$workspaceRoot/.joyride/scripts/ws_caller.cljs

(ns ws-caller
  (:require [user-lib :as lib]))

(lib/info-message "from caller")

Executing just the ws_caller script reports:
ERROR: Run Workspace Script Failed: ws_caller.cljs ENOENT: no such file or directory, open 'c:\wspath\.joyride\scripts\user_lib.cljs'

Executing user_lib first and then ws_caller works as expected (the information message is displayed).

My initial speculation based on a small amount of knowledge of sci is that if :load-fn looked in both paths, that would let users require namespaces/files from either location.

joyride/src/main/joyride/sci.cljs

Lines 34 to 44 in a9e29ec

	:load-fn (fn [{:keys [namespace opts]}]
	(cond
	(symbol? namespace)
	{:source
	(let [path (ns->path namespace)]
	(str
	(fs/readFileSync
	(path/join
	(utils/workspace-root)
	workspace-scripts-path
	path))))}

Presumably this would open up ambiguity around the search order, e.g. identically-named files in both places would lead to a decision needing to be made about which one(s) to load, potentially based on whether a ws or user script is being run.

[PEZ]

Thanks for this issue, @bobisageek! Is there something from the discussion on Slack that we should bring in here as well? For the benefit of whomever wants to tackle this.

[bobisageek]

My initial motivation was that I might have a number of functions that I want to re-use across workspaces (e.g. convenience functions around the VSCode API). borkdude made a great point, which is that there are interesting decisions around the relationship between workspace scripts and user scripts.

On one hand, the initial desire is to require user scripts from workspace scripts. But, workspace scripts can presumably be shared between users of a workspace, and other users might not have those same user scripts, which means the workspace scripts are potentially broken/incomplete in the context of another user.

So, the superficial ask is to search both paths for scripts to require. But there's sort of a matrix of possibilities:

• try to load the script from wherever it is
  • this would probably include the decision as to which path to check first, or to check both and error if there isn't exactly one, etc
  • this also has the potential of breakage described above
• try to load from workspace scripts when running a workspace script, and from user scripts when running a user script
  • this should keep everything 'complete' (no crossing between WS/user means everything should be 'portable')
  • the con to this is that some common code that gets used by WS scripts in different workspaces needs to be duplicated across those workspaces (I think?? there are maybe workarounds to this with soft links)
• decide on a hierarchy (e.g WS scripts can read from user, but not the other way, or vice-versa)
  • mainly just noted for comprehensiveness, I think this is the worst option, because it doesn't provide 'completeness', and could push users into workarounds

[PEZ]

I just ran across this trying to add an example User activate.cljs that would require a User my-lib namespace, that I then want to use from keyboard shortcut bindings. See this comment on a Calva issue for the use case.

The keybinding can be done like so:

    {
        "key": "cmd+f4",
        "command": "joyride.runCode",
        "args": "(require '[promesa.core :as p]) (p/do (vscode/commands.executeCommand \"calva.clojureLsp.stop\") (vscode/commands.executeCommand \"calva.clojureLsp.start\"))"
    },

I want to do it like so:

    {
        "key": "cmd+f4",
        "command": "joyride.runCode",
        "args": "(my-lib/restart-clojure-lsp)"
    },

Because in my_lib.cljs I have a REPL. And because hard to read and write code in JSON strings.

For this my plan was to have this in User activate.cljs

(ns activate
  (:require ...
            [my-lib]
            ...))

And the following User mylib.cljs:

(ns my-lib
  (:require ["vscode" :as vscode]
            [promesa.core :as p]))

(defn restart-clojure-lsp []
  (p/do (vscode/commands.executeCommand "calva.clojureLsp.stop") 
        (vscode/commands.executeCommand "calva.clojureLsp.start")))

But... running the User activate.cljs script, gives me:

User activate script: /Users/pez/.config/joyride/scripts/activate.cljs.  Running...
ERROR: Run User Script Failed: activate.cljs ENOENT: no such file or directory, open '/Users/pez/Projects/joyride/examples/.joyride/scripts/my_lib.cljs'

[bobisageek]

I was playing around with this a little bit today, and I had an idea, but I'm not sure if I actually like it, or if it's just appealing because it's sort of novelty.

The basic jist is that (require 'foo) will look for foo.cljs in either the user or workspace script directory. If we find exactly one match, load it basically the same way that it currently gets loaded. If we find no matches, we utils/say-error that we couldn't find the script, and nothing gets loaded. If we find more than one match, this is where it gets fun. My experimental implementation errors and says that we found more than one file.

With just this much, there's a sort of unrecoverable situation - a script with the same name in both the user and workspace directories means we can't require either. The idea that I'm not sure if I like or not is allowing for constraining the search with :from. So, given a user directory with 'user' and 'both' scripts, and a workspace directory with 'ws' and 'both' scripts, the following outcomes would be possible:

(require 'user) - loads user script
(require 'ws) - loads ws script
(require 'both) - error - ambiguous require
(require '[both :from workspace]) - loads workspace script
(require '[both :from user]) - loads user script

Requiring a script :from user that only exists in the workspace or vice-versa would also be an "error - not found".

To borkdude's original point, this still leaves open the possibility of scripts breaking with sharing - a workspace script that requires a user script will break (or even worse, have different semantics) when run by a different user. With that said, it allows people to "cross the streams" if they're the only one using the workspace, to require user scripts from user scripts, and to constrain the 'load path' to prevent/fix the situation where there's a user script and a workspace script with the same name.

There is a very experimental implementation of this in bobisageek@d4b8256. This is not heavily tested and probably not real-world ready - just a proof of concept for feedback on the general idea.

Tangentially, there's a change to config.cljs in that commit that I think might be generally helpful - app-db isn't dereffed in workspace-abs-scripts-path, which was sort of messing with me for a while in the way I was using it. It looks like it's currently only used to load the workspace activate script, and that doesn't seem to be working on my setup without some changes to life_cycle.cljs. I'm not sure if that's a platform difference (I'm on Windows) or a potential issue to be dug into deeper, but I don't want to complect the two concerns.

[PEZ]

I'll have a closer look tomorrow, but even a quick read made me curious. Sounds very interesting.

I've fixed that deref bug in #60, btw. It messed with me something crazy!

[PEZ]

About :from. I don't know where I stand! On the one hand it looks like a very apt addition. On the other hand, should we be introducing new ns form syntax? WDYT, @borkdude? (We could consider using :in to not confuse with from in JS/TS.)

We could just go with detecting the conflict and giving up with an error complaint. Then the user can solve it by renaming one or the other namespace.

A nice thing with this approach to not try to prioritize the ”current” script section, is that it will work when there is no such current section. Like in my example above, where the joyride.runCode command is used.

[borkdude]

On the other hand, should we be introducing new ns form syntax?

Please not! :)

After having thought about this more, I think we should simply do this:

The "classpath" order is workspace:user.

So if you require and the namespace is available in the workspace, we load that. The workspace namespace overrides the user namespace. If it isn't there, we load the user script.

That's pretty much how you would expect it in normal Clojure too: project-local code first, then global code (in ~/.clojure/deps.edn).

Yes, this could lead to conflicts, but let's trust the user that they will take that upon themselves (like you can have conflicts in ~/.clojure/deps.edn).

[PEZ]

The "classpath" order is workspace:user.

I like it. It also takes care of the runCode case in a very predictable way.

Want to PR it, @bobisageek?

[bobisageek]

The transition to promise chaining is frying my brain a little bit, so you might be able to knock it out more quickly (and better). I'll keep playing around in the meanwhile.

[borkdude]

Wait... we don't have to transition to chaining promises. Where did that come from?

[bobisageek]

Well, moving into promise land, anyway. I think using vscode's fs implementation to handle remotes means promises. I need to play around with promesa to figure out how to best organize the logic (I'm used to being spoiled by Clojure). Or maybe I'm just totally off the beat again.

[borkdude]

I think using vscode's fs implementation to handle remotes means promises.

I don't see that as part of this issue, let's keep the scope of this issue to the require + workspace + user dir problem.

[borkdude]

Unless I'm missing a detail here. Do you mean that you want to read a file but vscode will only let you do this asynchronously? Then how were we doing this before?

[bobisageek]

I was basing that off this:
bobisageek@d4b8256#r73678735

We were (essentially) constructing a path and if it didn't exist, that failed in VSCode-space. The experimental implementation checks for existence (using node's synchronous fs), and my impression was that the ask from @PEZ was to use VSCode's fs for checking existence and for reading the file.

[borkdude]

If we're going to do that, then we would need to move to https://github.com/babashka/sci/blob/master/doc/async.md which is way beyond what this issue is about.

[PEZ]

We will need to do it eventually. But I agree the current issue is not about the fs thing, even if it touches that code. I'll open a new issue about it.

[borkdude]

@PEZ Sounds good. I think that would be an issue for me to work on, so I can co-develop the SCI async API while doing that.

[PEZ]

Issue created:

• File loading is not VS Code Remote compliant #61

So we agree that when fixing this issue, and checking for file existence, we continue to do it with the sync tools from node fs. Are you up for it then, @bobisageek ?

[bobisageek]

Sorry about the delay - I got distracted with lunch. A first cut of a PR is in so we maybe have a baseline upon which to improve.