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

Manage JVM vs. CLJS dependencies better #43

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Manage JVM vs. CLJS dependencies better #43

merged 2 commits into from
Nov 12, 2024

Conversation

seancorfield
Copy link
Member

@seancorfield seancorfield commented Nov 12, 2024

Fixes #42 by:

  • use Java Time in README examples
  • removes clj-time as a dependency
  • moves js-joda to a :cljs-test only dependency
  • kongeor/cljs-test-runner is behind olical/cljs-test-runner so switch to the latter
  • update build-tools
  • update GH workflow dependencies
  • ignore Calva REPL file
  • remove unnecessary cljc_test file (both clj and cljs test runners file the tests just fine)
  • use clojure.math in cljs and clj
  • fix the bad cljs-only test splice

Signed-off-by: Sean Corfield [email protected]

* use Java Time in README examples
* removes clj-time as a dependency
* moves js-joda to a :cljs-test only dependency
* kongeor/cljs-test-runner is behind olical/cljs-test-runner so switch to the latter
* update build-tools
* update GH workflow dependencies
* ignore Calva REPL file
* remove unnecessary cljc_test file (both clj and cljs test runners file the tests just fine)
* use clojure.math in cljs and clj
* fix the bad cljs-only test splice

Signed-off-by: Sean Corfield <[email protected]>
@seancorfield
Copy link
Member Author

The cljc_test.cljc file caused errors when clojure -M:cljs-test was run (window not defined) and it really doesn't add much to the Chrome/Firefox test output on Karma (I've never seen anyone bother to test that way, beyond play clojure -M:cljs-test).

Copy link
Collaborator

@kongeor kongeor left a comment

Choose a reason for hiding this comment

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

Thank you Sean. Looks good!

user> (require '[clj-time.core :as t])
nil
user> (import '(java.time LocalDateTime) '(java.time.temporal ChronoUnit))
java.time.temporal.ChronoUnit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both of these imports are used so I'm not sure why you think this is redundant?

@hlship hlship added this to the 1.1 milestone Nov 12, 2024
@hlship hlship changed the title fixes #42 by using Java Time in README Manage JVM vs. CLJS dependencies better Nov 12, 2024
@seancorfield seancorfield merged commit 57d871b into master Nov 12, 2024
1 check passed
@seancorfield seancorfield deleted the issue-42 branch November 12, 2024 16:25
@hlship
Copy link
Collaborator

hlship commented Nov 12, 2024

Thanks for this; sorting out the JVM vs. CLJS stuff was something outside my wheelhouse as I don't do much with CLJS.

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.

clj-time is deprecated; README should use Java Time instead
3 participants