-
Notifications
You must be signed in to change notification settings - Fork 25
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
cljs fixes #106
cljs fixes #106
Conversation
@pkpkpk This looks good to me. Lmk when you want to have it merged. Also, are these changes covered by CI/CD tests for cljs? |
I want to cut a fress release to silence some warnings and then tweak the tests here to run on advanced builds, do a little more in the browser tests. @whilo Ill make those changes and then ping you on slack to help get CI running |
In Summary
./bin/install
./bin/run-all future work
|
That sounds good. Is this ready for review/merging? @pkpkpk |
Yes I think it's in a good place. Will revisit record handling safety in cljs another time |
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.
It looks solid overall. Thank you so much for turning the cljs support robust! I will merge this now to test the CI. We should also update the README to point to your implementation notes, or better, merge them. So maybe a follow up PR will be necessary.
No description provided.