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

npm dependency is culled out when I run lein dev #128

Closed
Turmolt opened this issue May 10, 2020 · 20 comments
Closed

npm dependency is culled out when I run lein dev #128

Turmolt opened this issue May 10, 2020 · 20 comments
Assignees

Comments

@Turmolt
Copy link

Turmolt commented May 10, 2020

I am trying to add quil to my new project, but every time I lein dev it seems to remove p5 from my node_modules folder and package.json before trying to compile and telling me

The required JS dependency "p5" is not available, it was required by "cljsjs/p5.cljs".

It is easy to repeat:

  • lein new re-frame myproject
  • add [quil "3.1.0"] to dependencies
  • npm i p5 and check that it did indeed install p5 in the node_modules folder
  • lein dev and watch the p5 folder get removed and then it throws the above error

I tried manually adding "p5": "1.0.0" to the package.json but that didn't seem to do anything and it was removed after trying to compile.

EDIT: I did a little more digging and found this lein-shadow example:
https://gitlab.com/nikperic/lein-shadow/-/blob/master/examples/lein-shadow-example/project.clj

Following their example I added :npm-deps [[p5 "1.0.0"]] to my project.clj and it still seems to cull it out.

@Turmolt Turmolt changed the title P5 is culled out when I run lein dev npm dependency is culled out when I run lein dev May 10, 2020
@superstructor
Copy link
Contributor

Please put NPM dependencies in src/cljs/deps.cljs; e.g.

{:libs                   ["some-google-closure-ns-lib.js"]
 :npm-deps        {"pako" "1.0.10"}
 :npm-dev-deps {"shadow-cljs"                    "2.8.110"
                           "karma"                              "4.4.1"
                           "karma-chrome-launcher" "3.1.0"
                           "karma-cljs-test"                "0.1.0"
                           "karma-junit-reporter"       "2.0.1"}}

deps.cljs is the standard used by ClojureScript, and by shadow-cljs for transitive dependencies.

:npm-deps is package.json/"dependencies". For the current project lein-shadow will write this into package.json. For transitive dependencies (i.e. dependent project) shadow-cljs itself (not lein-shadow) will look in the classpath for :npm-deps in deps.cljs files.

:npm-dev-deps is package.json/"devDependencies". For the current project lein-shadow will also write this into package.json. Dev dependencies are intentionally non-transitive (i.e. only for the current project).

For reasons not yet explained @thheller appears to recommend managing transitive dependencies as library authors with deps.cljs, but non-transitive direct dependencies as application authors with package.json. In situations where you need both, such as a library that has direct dependencies for tests or a benchmark app, this discrepancy creates duplication and messy conflicts. lein-shadow makes this situation consistent and deps.cljs the only source of truth.

We could do better at documenting this and the reasoning behind it, for which I will create a new issue.

On a related note @thheller, this issue was raised as part of a conversation on Clojurians Slack where you recommended not using lein-shadow. As far as I know there is no concrete information or discussion on why this recommendation exists ? If you want to make the case for that, please open an issue with the justification behind it so we can have a evidence-based discussion about it.

@superstructor
Copy link
Contributor

superstructor commented May 10, 2020

I have added some much needed documentation on this:
https://github.com/day8/re-frame-template#how-to-add-dependencies

@thheller
Copy link
Contributor

I thought I had made my stance on lein-shadow clear before but that may have been on Slack so it is probably lost. Let me restate it here so we have something that stays arround.

The thing that bothers me the most about lein-shadow is keeping the config that is supposed to be in shadow-cljs.edn in project.clj. shadow-cljs actually watches shadow-cljs.edn so if you make a build config change it will automatically reconfgure the build without requiring a restart. That feature is gone since it doesn't watch project.clj.

lein-shadow also goes directly against most documentation out there. I already helped two people by telling them to make changes in shadow-cljs.edn but them having the changes mysteriously disappear because of lein-shadow. A beginner struggling might ignore a warning about not editing a file when I'm telling them to do exactly that. In both cases they didn't tell me they were using lein-shadow because they didn't know and I only figured it out after a long line of questioning. This is time wasted on both sides. There should be one way to configure shadow-cljs and that should be shadow-cljs.edn. I see absolutely no gain in combining two config files into one, let alone inventing new commands for everything (eg. lein dev) over the regular documented commands.

Regarding npm dependencies:

There should be ONE source of truth for npm dependencies which should be package.json and managed by npm (or yarn). They provide useful features on top of editing package.json for you that users should be able to take advantage of (eg. yarn outdated). There are other useful features in package.json (eg. scripts) that users may like to take advantage off but can't since their additions are destroyed.

The reason deps.cljs is supported is that there must be a way for CLJS libraries to declare npm dependencies. We can't do that over package.json since we can't make npm/yarn look into .jar files. shadow-cljs installs the declared :npm-deps ONLY if they are not already in package.json. It will never overwrite/delete anything in package.json. There are already a couple different CLJS libraries with react dependencies wanting different versions. The user has to resolve these conflicts and the resolution mechanism is package.json.

Library publishing is not a topic handled by shadow-cljs currently so the publishing aspects of deps.cljs aren't handled. I admit that it is not ideal to have the information in two places but it isn't the end of the world. If I would write such a publishing tool I would take the information out of package.json and generate a deps.cljs, not the other way arround.

Overall I find this "stuff everything into project.clj" approach frustrating and confusing. It doesn't actually make anything easier. Just more confusing since there are now multiple ways to do everything, instead of just one. IMHO, YMMV.

its still not clear why the recommendation you propose is better for common use cases for ClojureScript devs

A good chunk of CLJS devs are coming from the JS world these days. They are more familiar with npm/yarn than project.clj. npm install a-thing is easy for them, editing project.clj is not.

I completely understand if the template wants to be targetted towards people that actually prefer lein and would like to stick with this workflow and rather not deal with npm/yarn. It is a lein template after all.

@thheller
Copy link
Contributor

One other usability issue I just realized that could be considered a bug.

On startup lein-shadow will reset package.json and run npm install for the directly declared :npm-deps. That wipes out all other packages that may have been there. Now shadow-cljs starts, looks at the deps.cljs in libs and checks against package.json and installs the missing ones. They are all missing since lein-shadow removed them all. This repeats for every fresh start adding a significant delay to the startup that isn't necessary.

@superstructor
Copy link
Contributor

We felt a little wedged between two worlds. @thheller we understand that you have a npm (or yarn) centric view, especially for new users coming from Node.js. We're coming from the other direction as seasoned Clojure devs so we have a lein centric view of the world. We really see npm as nothing more than a convenient source of JavaScript packages and want to use it as little as possible. We also want to minimise the support load for @thheller and ensure that beginners have the best possible experience.

After sleeping on it, we've made some changes. We've changed practically everything except the location of shadow-cljs.edn which is, by design, still in project.clj.

Our greatest concern at the moment is to completely eradicate the support overhead that you get from this @thheller As you are doing a wonderful job on shadow-cljs and we don't want you wasting another second on support associated with these kind of issues.

If these changes continue to not work and cause pain, we will look at more drastic changes. But we think this achieves a good balance and has got a good chance of solving the issues 100%.

On the other hand, as Clojure devs with complex commercial projects, lein delivers a bunch of features that we do not get with shadow-cljs. E.g. computation to build the compiler configuration (shadow-cljs.edn is static, not .clj), middleware, plugins, lein-git-inject etc. This is even more important when there is both frontend and backend involved, such as the luminus template which also uses lein-shadow. So we feel as if it is a natural thing for a ClojureScript project to embrace Leiningen (or Boot, or Clojure Deps etc, but we are only talking about lein-shadow here after all). Certainly, our commercial projects are not about to move away from using Leiningen.

We've added much more aggressive commenting to shadow-cljs.edn so it will be unmissable that you are not supposed to edit this file when using lein-shadow - https://gitlab.com/superstructor/lein-shadow/-/commit/01050d682902486dbbf612a10f3a396d284387e3#97252e6c66e87cb8504c0ecda1ec59d8a0c029d3_13_11

On startup lein-shadow will reset package.json and run npm install for the directly declared :npm-deps. That wipes out all other packages that may have been there. Now shadow-cljs starts, looks at the deps.cljs in libs and checks against package.json and installs the missing ones. They are all missing since lein-shadow removed them all. This repeats for every fresh start adding a significant delay to the startup that isn't necessary.

Thanks for pointing this out, it wasn't ideal. So now, we don't touch package.json at all and interop instead with the npm command i.e. npm install --save .... This also fixes the issues with overwriting package.json, being able to use npm features like npm outdated etc.

More details of further changes are on the lein-shadow PR on GitLab (or MR in GitLab parlance).

@thheller
Copy link
Contributor

I have never advocated for moving away from lein. I don't recommend using it for CLJS-only projects but I can absolutely understand the use in mixed CLJ/S projects. I continue to use it myself, albeit sans lein-shadow.

I will however continue recommending to move away from lein-shadow and I will continue to do so as long as shadow-cljs.edn contents stay in project.clj. I'm not going to change my opinion on that front unless I see some actual examples of what lein-shadow allows that shadow-cljs.edn doesn't. So far I only see lost features and nothing gained. Require a restart for every build config change? Seriously? That reason alone is enough for me to never recommend lein-shadow.

My goal with shadow-cljs is to provide a CLJS-focused development experience. lein was built for CLJ and is optimized for that. Some of that applies to CLJS some of it just gets in the way. I wrote about some of the reasons why the shadow-cljs CLI even exists before. I have added optimizations since and will continue to do so. I have plans so that changing :dependencies no longer requires a restart, that will however never work in any embedded scenario where shadow-cljs is not in charge of the classpath.

I'm tired of pretending that CLJS is exactly like CLJ. It isn't. My workflow is completely different for CLJS than it is for CLJ. That is why there are different tools. I absolutely understand if people with a CLJ-centric workflow want to stay in that and rather not leave. Just saying that you might be missing out by doing so.

Let me give you another example of a workflow that I'm accustomed to that becomes several times slower when using lein-shadow. Lets for the sake of the argument assume that it takes 10sec to get shadow-cljs started via lein-shadow. Might be faster on some machines, might be slower on others. Point is there is a cost to starting it. That cost is halved when using shadow-cljs.edn because it defaults to using an AOT release but that is a topic for another day.

In my workflow I start shadow-cljs server once and leave that running until I need to change :dependencies. There should be no reason to ever do it for any other reason. I consider any other reason a bug. So 10sec once every few days when the project is settled, maybe more often in the beginning when gathering deps.

I then start whatever I need to when I need to. I do so from the UI but the command line is fine too.

$ shadow-cljs watch app
# edit shadow-cljs.edn to configure a :dev-http I forgot
# edit shadow-cljs.edn to modify build config because I forgot a :preloads
# CTRL+C, the watch because I want to try a release build
$ shadow-cljs release app
# ooops, seems to have some externs issue
$ shadow-cljs release app --debug
# oops forgot to add :compiler-options {:infer-externs :auto}
$ shadow-cljs watch app
# fix warnings, CTRL+C
$ shadow-cljs release app
# verified it works, generate report
$ shadow-cljs run shadow.cljs.build-report ...
# remove leftover cljs.pprint require, verify again
$ shadow-cljs run shadow.cljs.build-report ...
# looks solid, time to commit/publish/whatever

Phew done. I don't know about you but I did these exact same steps last week on a fresh project. Amount of times I had to wait for stuff to actually start doing stuff? Once, the server startup. Everything else used the server. With lein-shadow you have to add an extra 9 times where you are waiting for things to start. I don't know about you but I get easily distracted when waiting for stuff, so that can easily becomes hours. 10s feels like an eternity.

IMHO, YMMV.

@superstructor
Copy link
Contributor

some actual examples of what lein-shadow allows that shadow-cljs.edn doesn't

How do you do

 :compiler-options {:closure-defines {my-app.core/some-def ~some-dynamic-var}}

in shadow-cljs.edn @thheller ?

This is a core issue, if we can't use build code (e.g. lein plugins) to define closure defines then we can't move shadow-cljs.edn out of project.clj.

If there is a way to address this, then please share it because I'd genuinely be happy with a more cohesive ecosystem where lein-shadow is recommend for lein users.

@superstructor
Copy link
Contributor

I've released re-frame-template v1.0.34 with the changes as although we havn't reached an understanding on shadow-cljs.edn yet the changes do fix the issues related to package.json including support requests arising on Clojurians Slack as recently as a few hours ago.

I look forward to your feedback on the above example of dynamically closure defines @thheller There are other examples, but for ease of discussion lets do one at a time.

@thheller
Copy link
Contributor

Well, first of all I'd ask for the nature of the closure-defines you are setting this way. Too often I see people turning what should be runtime configuration into compile time constants. I'm not saying there aren't any use-cases just typically fewer than people make it out to be.

Anyways my recommended version would be to use --config-merge.

From the command line you might do

$ shadow-cljs release app --config-merge '{:closure-defines {my.app/foo "something"}}'

For more complex things I recommend using clj-run or just lein run and actually writing a bit of clojure to do everything. The Clojure API also allows passing in :config-merge via.

(require '[shadow.cljs.devtools.api :as shadow])
(shadow/release! :app {:config-merge [{:closure-defines {'my.app/foo "something"}}]})

You can go further with the CLJ API but typically :config-merge should do everything you need it to do. This might even be a case where it may make more sense for lein-shadow to go through the CLJ API directly instead of the command-line interface.

@superstructor
Copy link
Contributor

Thanks @thheller config merge and the CLJ API appears to be a good way forward.

If lein-shadow used the shadow-cljs cli and/or the CLI API with config merge as the interface, and left shadow-cljs.edn alone, would you be open to recommending lein-shadow ?

@thheller
Copy link
Contributor

Recommending it is unlikely but I'll probably stop trying to convince people to move away from it. ;)

I'd rather not get into a debate over why people shouldn't be putting .js sources in .jar files or serving anything from the classpath for that matter. I don't and I'm completely aware that my approach isn't what most people do today. I'm fine with lein-shadow being the "bridge" to bring CLJS output into the release uberjar. When shadow-cljs.edn exist people are free to decide if they want to use shadow-cljs directly or go through lein-shadow during development.

@superstructor
Copy link
Contributor

Using config merge and probably the CLJ API are the right way forward 👍 In this way shadow-cljs.edn will contain the bulk of the config and project.clj can be used to inject additional parts for individual use cases. I'll make the changes to lein-shadow and re-frame-template at some point, just havn't had time to look at it yet. @thheller

@thheller
Copy link
Contributor

I have been thinking about adding an additional config layer. In teams it is not uncommon that different people have different preferences so I want individual devs to be able to override certain settings based on their preferred environment.

Not sure how that would align with lein-shadow but I'm thinking about allowing a shadow-cljs.local.edn config that each user could have in their project and keep out of git. Or maybe shadow-cljs.<username>.edn. Some settings in there would override whatever is in shadow-cljs.edn, much like --config-merge.

One question yesterday was about disabling devtools for a :node-script build since one dev doesn't use the REPL but the others do.

So

shadow-cljs watch script --config-merge "{:devtools {:enabled false}}"

would become shadow-cljs.local.edn

{:nrepl false
 :builds {:script {:devtools {:enabled false}}}

Which would also never start the nrepl server which is something the --config-merge can't do since it works on the build level only.

You could then generated a shadow-cljs.lein-shadow.edn or so and when calling shadow-cljs pass the extra --profiles=lein-shadow argument to have that merged in? Or the equivalent in the CLJ api?

@superstructor
Copy link
Contributor

Thanks @thheller

If I understand correctly the only significant difference between this proposed additional layer and config-merge is that config-merge works at the build level only ?

I've reviewed a lot of our configs for potential use cases and this additional layer would come in useful as one of the use cases for lein-provided config is lein generating build-ids and the associated config map for the build.

Either way, I'll make lein-shadow work with the API(s) provided for config as its the right way forward to leave shadow-cljs.edn to shadow-cljs and package.json to NPM.

@superstructor superstructor self-assigned this Jun 26, 2020
@mike-thompson-day8
Copy link
Contributor

mike-thompson-day8 commented Jul 6, 2020

Okay, so we made a series of changes about six weeks ago with the goal of eradicating the support issues for Thomas arising from beginner use of this template.

And I've been monitoring #shodow-clj channel on clojurians since that time and the good news is that the support issues seem to have virtually disappeared for Thomas. In fact, I don't think I have seen a single issue whereas before there was at least 2 or 3 a week coming through.

So, at that level, I think we can say we have succeeded.

We now have to find time to go the next step and use the --config-merge technique which Thomas revealed (above) but that might take longer.

@superstructor
Copy link
Contributor

@thheller Have looked into this again. To do it in a way that solves current use cases would need an API such as your proposed profiles that worked at the entire config level, not just the build level only ala --config-merge.

Looked through code on master as of today and looks like only --config-merge is available at the moment ? Whats your current thinking on adding a wider scoped config option ?

@thheller
Copy link
Contributor

thheller commented Aug 3, 2020

I have not thought about it any further. I don't know what is needed, we only talked about build-level options so far.

@superstructor
Copy link
Contributor

@thheller Re your earlier comment you mentioned thinking about a --profiles=lein-shadow argument that merged in shadow-cljs.lein-shadow.edn file.

The key difference here --config-merge merges into the build-id.

What I need is a way to merge into the entire shadow-cljs.edn file.

It doesn't necessarily need to be from another file. It could just take a string of EDN like --config-merge.

Thoughts ?

@thheller
Copy link
Contributor

thheller commented Aug 7, 2020

No thoughts until I get an actual problem statement. What do you want to merge?

I need exact descriptions since not every attribute in shadow-cljs.edn has the same "rules". Some may apply once when the server starts and some may be "watched" and actually cause changes at runtime when modified. For --config-merge I ensured that even the overrides still apply when the build config is changed. There is no such functionality for the "global" config so I need to a little more concrete info for what you are actually trying to do.

The --profiles stuff was an idea I have not thought about further and I'm kinda hesitant to add since it makes things a lot more complicated overall. Not only for shadow-cljs itself but also for tools like Cursive which may one day want to support shadow-cljs.edn. Adding a bunch of config magic will make that take much longer than a simple EDN file.

@superstructor
Copy link
Contributor

As of v2.0.0 released today we have entirely replaced Leiningen with a pure shadow-cljs build.

So now,

  • lein-shadow no longer manages dependencies; so you have to use npm or yarn directly.
  • lein watch becomes npm run watch
  • lein shadow --help becomes npx shadow-cljs --help

Closing as this should resolve this and all related issues.

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

4 participants