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

Handle string requires gracefully #22

Closed
vemv opened this issue Aug 29, 2021 · 10 comments · Fixed by #25
Closed

Handle string requires gracefully #22

vemv opened this issue Aug 29, 2021 · 10 comments · Fixed by #25

Comments

@vemv
Copy link
Member

vemv commented Aug 29, 2021

Context

https://clojurians.zulipchat.com/#narrow/stream/180378-slack-archive/topic/cider/near/251080205

Brief

has a :pre that won't play fine with shadow-cljs "string requires", leading to client errors whenever one of those requires is present.

Suggestion

If a ns is a string, instead of computing (meta (clojure.core/find-ns ns)), return {}.

Similar for any other place where "string requires" might be handled throughout the codebase

...Obviously, ideally clj-suitable would understand the "string requires" and offer actual functionality for them. Not sure if that's at hand though. So it seems sensible to at least avoid a hard failure.

@nwjsmith
Copy link

nwjsmith commented Sep 3, 2021

Just ran into this, although I'm not using shadow-cljs. ClojureScript's Library Property Namespaces can require string requires as well.

@vemv
Copy link
Member Author

vemv commented Sep 13, 2021

Released [org.rksm/suitable "0.4.1-alpha1"], feel free to try it out

@vemv
Copy link
Member Author

vemv commented Sep 13, 2021

Whoops, sorry, it's not as easy as bumping that dep isolatedly. We have to cut a cider-nrepl release that uses the new suitable.

@nwjsmith
Copy link

@vemv is there a way to override the version of suitable that cider-nrepl depends on in my local setup? I'd like to test this out if possible.

@vemv
Copy link
Member Author

vemv commented Sep 30, 2021

Not directly, however we can release cider-nrepl which you could refer to in cider.el

Relatedly, @bbatsov WDYT of making this defconst a defcustom instead? https://github.com/clojure-emacs/cider/blob/adfc1c940d452ace7d1dbea543024d422f60364c/cider.el#L409 Maybe we can split it into two as you mentioned earlier today.

(I'd suggest not introducing a micro breaking change with it)

@bbatsov
Copy link
Member

bbatsov commented Sep 30, 2021

Btw, constants in Elisp are not really constants - you can set this to whatever you want in your setup. And yeah - I'll likely add a defcustom for the injected cider-nrepl version.

@bbatsov
Copy link
Member

bbatsov commented Sep 30, 2021

Just made the necessary changes - see cider-injected-middleware-version.

@vemv
Copy link
Member Author

vemv commented Oct 2, 2021

Fix finally available in cider-20211002.1153.tar (i.e. today's CIDER snapshot published on MELPA) or via [org.rksm/suitable "0.4.1"] for other setups

@nwjsmith

@nwjsmith
Copy link

nwjsmith commented Oct 6, 2021

@vemv just tried it out and it's working great. Thanks for all your work on this.

@vemv
Copy link
Member Author

vemv commented Oct 6, 2021

Very happy to read that! 🍻

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 a pull request may close this issue.

3 participants