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

redudant codemirror dependency #1

Open
ikappaki opened this issue Mar 26, 2024 · 2 comments · May be fixed by #2
Open

redudant codemirror dependency #1

ikappaki opened this issue Mar 26, 2024 · 2 comments · May be fixed by #2

Comments

@ikappaki
Copy link

Hi,

the command-bar library is dependent on codemirror at this early stage, in what appears to be code to support the demo.

(:require ["@codemirror/state" :refer [StateField]]
["@codemirror/view" :refer [keymap EditorView ViewPlugin]]
[applied-science.js-interop :as j]
[clojure.string :as str]
[nextjournal.clerk.render.hooks :as hooks]
[nextjournal.command-bar.fuzzy :as fuzzy]
[nextjournal.command-bar.keybind :as keybind]
[reagent.core :as reagent]))

This means that early adopters for the lib will bring in an additional unnecessary dependency in their app, likely to cause conflicts if they also have a local codemirror setup, other than the unused code they bring in.

A solution is to relocate the relevant code to the demo.

PR to follow.

@ikappaki ikappaki linked a pull request Mar 26, 2024 that will close this issue
@zampino
Copy link
Collaborator

zampino commented Mar 26, 2024

This means that early adopters for the lib will bring in an additional unnecessary dependency in their app, likely to cause conflicts if they also have a local codemirror setup, other than the unused code they bring in.

I'm not sure I follow. Since clerk render deps are in the main profile, if shadow-cljs resolves dependencies correctly you will still have codemirror deps pulled in, as per your remark in #2

the lib does not require any additional npm deps other than the one already defined in clerks deps.cljs.

so in theory we could just get rid of the deps.cljs in this repo.

Also I'm not sure this library was planned to be used outside of a codemirror context, but I'd rather wait for @philippamarkovics to take a look at this

if they also have a local codemirror setup,

are you referring to a setup which is not involving a shadow build, like es module imports? Furthermore I think shadow will need those codemirror deps at build time for compiling clojure-mode which is also part of the main deps.

@ikappaki
Copy link
Author

ikappaki commented Mar 26, 2024

This means that early adopters for the lib will bring in an additional unnecessary dependency in their app, likely to cause conflicts if they also have a local codemirror setup, other than the unused code they bring in.

I'm not sure I follow. Since clerk render deps are in the main profile, if shadow-cljs resolves dependencies correctly you will still have codemirror deps pulled in, as per your remark in #2

You are right, I had a problem with the @codemirror/view dependency injected in my package.json that prevented nextjournal/lang-clojure from working when defined as a codemirror extension. Initially, I thought the problem was limited to the command-button's deps.cljs. However, you correctly noted that the same dependency is also referenced in the clerk's rendering component.

The problem was resolved by removing the injected @codemirror/view, lezer/common, and lezer/highlight dependencies from the test application's package.json. The rationale behind this resolution remains unclear to me, having been deduced through trial and error.

the lib does not require any additional npm deps other than the one already defined in clerks deps.cljs.

so in theory we could just get rid of the deps.cljs in this repo.

Yes, I believe so, though I've been cautious about implementing significant modifications.

Also I'm not sure this library was planned to be used outside of a codemirror context, but I'd rather wait for @philippamarkovics to take a look at this

if they also have a local codemirror setup,

are you referring to a setup which is not involving a shadow build, like es module imports?

I initially thought the issue with initializing a local codemirror instance in the test application stemmed from its use within the command-button, but this turned out to be incorrect as noted above.

Nevertheless, I believe the patch remains valid since it eliminates the unnecessary reference of an unused codemirror code.

Thanks

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.

2 participants