Support nbb as native cljs repl

[ikappaki]

Hi,

(while I was opening this PR, I've realised that @benjamin-asdf has arleady submitted #3272 to address the same using a more general methodology through the clj REPL. I don't think there is a source code conflict between the two. His PR appears to be allowing the clj REPL to upgrade to cljs and thus support a variaty of cljs cases, this PR is specific to nbb to support it as a native cljs REPL and goes through the established root of declaring a new project tool. Thus I provisionally open this PR for deliberation only and should be declined in favor of benjamin's if it conflicts in any way or is considered overlapping/obsolete)

Could you please consider PR to support nbb as a native cljs repl when jacking in or connecting to cljs. Addresses #3272

The logic will consider a project as nbb based on whether a package.json is present. A user can jack in using cider-jack-in-cljs or cider-connect-*-cljs

The previous logic assumed that all cljs REPLs are clojure repls that required some form to switch them over to cljs. With this update a particular project type can be declared as native cljs by not declaring any repl init forms.

I've included some tests to test the above.

It will be nice if we have integration tests to confirm we haven't broken anything when having such high level changes that cannot be unit tested (e.g. the jack in command), thus raised #3274 to discuss whether we can bring this forward.

I will also need to add nbb documentation if this is accepted. Thanks

---

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• All tests are passing (eldev test)
• All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

[bbatsov]

I like your proposal, as it's quite in line with how things are structured today in CIDER and it making it possible to use cider-jack-in-cljs without the hardcoded connection upgrade logic. I'll provide a bit more inline feedback, but I can tell from now that I'll accept this PR.

[bbatsov]

Actually the code is so good that I don't even have much feedback on it. :D Well done!

Now we need just a bit of docs and maybe those integration tests that you've suggested.

[ikappaki]

Now we need just a bit of docs and maybe those integration tests that you've suggested.

I would also like some feedback from @benjamin-asdf whether our patches overlap, I dont' think so, because his is more like a general approach to upgrade a clj to cljs which addresses a wider set of problems. If they overlap, I'd rather support his, since he was submitted the PR first.

@borkdude, I've used package.json as the project file to suggest this is likely an nbb project so that CIDER will either suggest using nbb or jack in to nbb without confirmation in abasense of any other project files. Is it a good choice of nbb "project file"? Also would be grateful if you look at the new nbb doc section and suggest if you need anything else to go into this (I've just referenced what was there already for babashka).

There is some change in general behaviour that I've noticed that I think it's good to mention for completion:
1. When jacking into a shadow-cljs with shadow-cljs.edn project (a primarly shadow project), CIDER will now ask the user whether they want to now use shadow or nbb (because of the package.json), instead of defaulting to shadow. (not relevant any more since project file identifier changed to nbb.edn).
2. For every cljs project, including nbb, CIDER will by default ask the user which repl type the user wants to use and present them with the whole list of supported repls (shadow-select, nbb etc etc). It has been always like this, but now it looks silly for CIDER to ask a user who they are just selecting nbb (either directly or indirectly), which cljs repl they want to use from the full cljs repl list, given that there is only one cljs type that works, and this is nbb. Perhaps a further general improvement would be to reduce the cljs repl types available to only those support by the project tool.

I have clojure cli, shadow cljs and nbb included in the regression testing, perhaps I can commit the integration tests first followed by this patch? I think i only need to add one more, leiningen,. in the list and that would be sufficient.

Thanks

[borkdude]

@ikappaki nbb.edn would be a good choice. I've never heard of project.json, do you mean package.json?

[ikappaki]

@ikappaki nbb.edn would be a good choice. I've never heard of project.json, do you mean package.json?

Yes sorry, package.json, is nbb.edn currently a thing being used?

[borkdude]

@ikappaki Yes :) You can declare deps in it, just like in bb.edn. No :tasks though. It requires bb to download the deps.

[benjamin-asdf]

@ikappaki It doesn't overlap I think. As long have the build tool commands it makes sense to add new ones I guess. So thanks! I still like my pr, or something like it, because I can connect to nbb, scittle, joyride with 1 command and I feel like that is a good direction to go.

[ikappaki]

@ikappaki Yes :) You can declare deps in it, just like in bb.edn. No :tasks though. It requires bb to download the deps.

Thanks @borkdude, changed project file identifier to nbb.edn. I've also crossed out the observation that jacking in to a shadow-cljs only project would also prompt the user to choose between shadow and nbb.

[ikappaki]

@ikappaki It doesn't overlap I think. As long have the build tool commands it makes sense to add new ones I guess. So thanks! I still like my pr, or something like it, because I can connect to nbb, scittle, joyride with 1 command and I feel like that is a good direction to go.

@thanks @benjamin-asdf, is good to know :)

[ikappaki]

I have clojure cli, shadow cljs and nbb included in the regression testing, perhaps I can commit the integration tests first followed by this patch? I think i only need to add one more, leiningen,. in the list and that would be sufficient.

Hi @bbatsov, now that the integration tests have been merged, can we have clojure-mode with nbb support released so I can resume work on this PR please? (as a reminder, we needed nbb support in clojure-mode to locate the root dir of the project)

[bbatsov]

I'm on the road right now without a computer. I can cut a new release tomorrow evening or on Monday.

…

On Sat, Dec 10, 2022, at 6:54 PM, ikappaki wrote: > I have clojure cli, shadow cljs and nbb included in the regression testing, perhaps I can commit the integration tests first followed by this patch? I think i only need to add one more, leiningen,. in the list and that would be sufficient. > Hi @bbatsov <https://github.com/bbatsov>, now that the integration tests have been merged, can we have clojure-mode with nbb support <clojure-emacs/clojure-mode#641> released so I can resume work on this PR please? (as a reminder, we needed nbb support in clojure-mode to locate the root dir of the project) — Reply to this email directly, view it on GitHub <#3275 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZLSVIDZPKWGI5GD5GSFLWMS7V5ANCNFSM6AAAAAASRVCUE4>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[ikappaki]

I'm on the road right now without a computer. I can cut a new release tomorrow evening or on Monday.

No worries, thanks, we are not in a rush :)

[bbatsov]

@ikappaki Done - https://github.com/clojure-emacs/clojure-mode/releases/tag/v5.16.0

[ikappaki]

Hi @bbatsov,

I think PR is complete with latest commit ready for review

1. Upgraded clojure-mode to 5.16.0 to pick up nbb support
2. Incorporated latest pending cljs changes from mainline and updated tests
3. Introduced nbb integration test

Thanks

[bbatsov]

Great work! 🙇‍♂️