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

Use npm-style names for JS libs #201

Merged
merged 3 commits into from
Feb 19, 2019
Merged

Use npm-style names for JS libs #201

merged 3 commits into from
Feb 19, 2019

Conversation

Deraen
Copy link
Contributor

@Deraen Deraen commented Apr 26, 2018

Partially fixes #188

The bundles Reagent should be updated to 0.8.0 and after that Re-frame-10x should work by default with Cljsjs, but also with ClojureScript npm-deps and with Shadow-CLJS.

I tested this with Cljsjs packages but not with other environments.

@danielcompton
Copy link
Contributor

danielcompton commented Apr 26, 2018

Thanks for putting this together. What's the minimum ClojureScript and Reagent versions that this will support? I don't really understand all of the subtleties going on here, but will be happy to put out a beta version so people can test it. Do we need to update the install docs, or is this all transparent to the consumer?

project.clj Outdated
@@ -38,7 +38,7 @@
cljsjs/create-react-class
org.clojure/tools.logging
net.cgrand/macrovich]]
^:source-dep [reagent "0.7.0"
^:source-dep [reagent "0.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two parallel release branches, one for reagent 0.7.0 and one for 0.8.0. Will this technique work for both versions of Reagent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can work for 0.7.0 because the Cljsjs libs also provide the old cljsjs.* names. But obviously that won't work with Shadow-CLJS.

What is the reason for separate branch? Why not just use 0.8 always?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Re-frame-10x uses bundled Reagent 0.8, everything would still work if user has Reagent 0.7 on the project? User will have React 15 dependencies on the project, but Reagent 0.8 should work with those, except for new React 16 features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for separate branch? Why not just use 0.8 always?

Until recently we were using Reagent 0.7 while Reagent 0.8 was in alpha. However users were using Reagent 0.8/React 16 so we created a parallel release branch for them to be able to use re-frame-10x.

@Deraen
Copy link
Contributor Author

Deraen commented Apr 27, 2018

This is transparent for Cljsjs users.

This probably works from ClojureScript 1.9.854 with Cljsjs packages, but I didn't test. With Node modules the latest version is required.

@danielcompton
Copy link
Contributor

Thanks very much for this. I don't have the time/brainspace to bring this in now, but I'll try and merge it soon and cut a release once we get some more time to work on 10x. Thanks!

@danielcompton
Copy link
Contributor

Sorry for the long time since I looked at this. Does the approach taken here still work, and is it still the best way to do so? If so, I'll get it merged. Don't worry about the conflicts if you don't want to, I can fix them.

Thanks!

@Deraen
Copy link
Contributor Author

Deraen commented Feb 19, 2019

Yeah, this is fine. I rebased the changes and tested this locally.

Should I create another PR for #224 or can I include the fix for that here also?

@Deraen
Copy link
Contributor Author

Deraen commented Feb 19, 2019

As mentioned in first comment, this should simplify using re-frame-10x with Shadow-cljs.

I don't exactly remember why I created deps.cljs but I think Shadow-cljs might be able to use that to automatically install npm deps.

@danielcompton
Copy link
Contributor

If you want to include a fix for #224 that would be great, if you have time, otherwise I can do it. Thanks!

@danielcompton danielcompton merged commit b2a34d7 into day8:master Feb 19, 2019
danielcompton added a commit that referenced this pull request Feb 19, 2019
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 this pull request may close these issues.

Investigate using :npm-deps to specify dependencies for Shadow CLJS users
2 participants