-
Notifications
You must be signed in to change notification settings - Fork 79
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
Canonicalize local path for URL consumption #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! Just have two minor remarks.
.nrepl-port
Outdated
@@ -1 +0,0 @@ | |||
.shadow-cljs/nrepl.port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an intentional link to .shadow-cljs/nrepl.port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry, I missed that I will revert, I'll just keep the esclusion in .gitignore since it kept coming up in the untracked files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I got my self confused. The reason I removed it is that when I do a cider-jack-in
into the clojure-cli
, the contents of that file change to a port number, thus the contents are always replaced with a new port number every time I jack in, and it always appear in git reports as a modified file.
How do you usually start a development environment in Clerk if not jacking in to clojure-cli
? I'm not familiar with redirecting the nrepl port number to shadow.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ikappaki the last part of the README, "Developing Clerk", explains how to start the dev environment using Babashka, after which you would cider-connect
to the port it prints to your shell. (Note that this is not required by projects that use Clerk, it has to do with starting some CLJS and CSS background tasks.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @jackrusher,
thanks! I have never managed to have bb dev
work for me since I've started looking at dev, other than bringing up the component library page and indulging myself looking at the various UI components available.
The first issue is that he viewers
version linked from the :sci
alias in deps.edn
is pointing to sha that has its modules/devdocs/deps.edn
pointing to clerk version with a :git/url
of git@github.com:nextjournal/clerk.git
. This means that it wont work for anyone other than those who have git access to the site. It has been subsequently updated in a recent viewers
commit (e06e36dc22ba88ed3852281e5a59de0e3b8c6a2d to be exact). Thus we need to get a new cut of the viewers
library to make it work.
Cloning: git@github.com:nextjournal/clerk.git
Error building classpath. Unable to clone C:\Users\chaos\.gitlibs\_repos\ssh\github.com\nextjournal\clerk
FATAL ERROR: No supported authentication methods available (server sent: publickey)
fatal: Could not read from remote repository.
I meant to be raising an issue for this. I'll do this now. I suppose I can't raise a PR because the particular viewers version to pick up is up to the maintainers to choose.
Then I'm getting a warning about cider and various middlewares not being in sync when i connect to the bb dev
shadow repl, that put me off so far looking at it (jacking in to clojure-cli directly worked great for me so far). I'll have a look at it next, the .nrepl-port
sym link comes up in CIDER btw while connecting, but it fails to connect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(raised #40 for the bb dev
issue)
src/nextjournal/clerk.clj
Outdated
"Opens `URL` in the system default browser." | ||
[url] | ||
|
||
(browse/browse-url url)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do without this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will revert.
I guess for these tests to be really useful it would be great to also run them on windows… |
Do you mind I setup an additional job to test on MS-Windows in the existing github workflow? This will be separate PR. Otherwise, it is more for Windows users to discover possible breaks by running the test suit on their local machine. Still useful but not as good as with the workflow integration. |
827a3ae
to
20d3232
Compare
(Reverted the (raised #41 for supporting MS-Windows in CI) |
20d3232
to
cd312e4
Compare
@ikappaki thanks a lot for this! |
Fixes #38. Co-authored-by: ikappaki <ikappaki@users.noreply.github.com> Co-authored-by: Martin Kavalar <mk@katercalling.com>
Hi,
could you please consider fix for #38.
What is happening here is that local Windows paths creep in in the URL which the browser does not treat as path separators. The fix is to convert any system path separators to forward slashes (so this should have no effect on Linux, convert
\\
to/
on MS-Windows, and support any unknown separators introduced in the future).I've bundled a test for this now that there is a test suite 🎉 Needed to upgrade
babashka.fs
to latest version to pick up thewith-temp-dir
api call.I've also removed and git ignored the .nrepl-port which I believe it was included by mistake.
All test pass both on MS-Windows and Linux, and also have managed to convert to static all tests in
clerk-demo
(under MS-Windows).Feel free to scrutinize the patch.
Thanks,