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

Shadow compat no longer necessary? #34

Open
vemv opened this issue Jul 22, 2023 · 6 comments
Open

Shadow compat no longer necessary? #34

vemv opened this issue Jul 22, 2023 · 6 comments

Comments

@vemv
Copy link
Member

vemv commented Jul 22, 2023

Given

https://github.com/thheller/shadow-cljs/blob/51b15dd52c74f1c504010f00cb84372bc2696a4d/src/main/shadow/cljs/devtools/server/nrepl_impl.clj#L46-L59

, I believe it's no longer necessary to have shadow-cljs specific code.

@bbatsov
Copy link
Member

bbatsov commented Jul 22, 2023

@vemv Can you elaborate on this? This bit of piggieback interface has existed in Shadow forever. (since before suitable was created)

@vemv
Copy link
Member Author

vemv commented Jul 22, 2023

Hmm, that was unexpected :)

If that bit has always existed, then why is shadow-specific code needed at all?

Maybe that bit doesn't work as intended?

I'll be looking at this, mainly because in cider-nrepl we only use piggieback-based cljs stuff. And some features doesn't appear to work.

So cider-nrepl would have to be fixed some way or another (fix the piggieback compat or introduce suitable-like compat to cider-nrepl code).

@bbatsov
Copy link
Member

bbatsov commented Jul 22, 2023

We don't use anything else just because I didn't want to bother to learn shadow's API and Tomas (the author of Shadow) was kind enough to offer this basic compatibility. :-) In an ideal world we use shadow's own nREPL middleware, as it provides full access to shadow's functionality. I recall not everything was working properly with the piggieback adapter, but I haven't touched the ClojureScript code in cider-nrepl in ages and I've started to forget the details.

@vemv vemv changed the title Shadow compat no longer necessary Shadow compat no longer necessary? Jul 22, 2023
@bbatsov
Copy link
Member

bbatsov commented Jul 22, 2023

See thheller/shadow-cljs#62 and thheller/shadow-cljs#561 Those are most useful discussions related to the nREPL support in shadow I could find now.

@vemv
Copy link
Member Author

vemv commented Jul 22, 2023

@vemv
Copy link
Member Author

vemv commented Jul 24, 2023

One thing I've just realised is that cider-nrepl and refactor-nrepl use cider.nrepl.middleware.util.cljs/grab-cljs-env, suitable doesn't

Perhaps using it here would simplify some code.

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

No branches or pull requests

2 participants