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

Extend protocols to JS built ins using CLJS "type symbols", fix extending protocols to default & Object #784

Merged
merged 13 commits into from
Jul 27, 2022

Conversation

lilactown
Copy link
Contributor

@lilactown lilactown commented Jul 25, 2022

Please answer the following questions and leave the below in as part of your PR.


Fixes #781, #782 and #783

:cljs (or (= 'object sym)
(= 'default sym))))
:cljs (= ::extend-default sym)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

even in the impl before this PR, this would AFAICT never happen because it wasn't possible to extend a protocol to either 'default or 'object

@lilactown lilactown changed the title Lilactown/cljs type symbols Extend protocols to JS built ins using CLJS "type symbols" + fix extending protocols to 'default Jul 25, 2022
simplifies the code when extending the protocol impls, and also fixes the
problem with extending to Object in clojure not returning true for satisfies?
@lilactown lilactown changed the title Extend protocols to JS built ins using CLJS "type symbols" + fix extending protocols to 'default Extend protocols to JS built ins using CLJS "type symbols", fix extending protocols to default & Object Jul 25, 2022
@borkdude
Copy link
Collaborator

@lilactown I tried this with this PR checked out locally:

cljs.user=> (sci/eval-string "(defprotocol IFoo) (extend-type string IFoo) (satisfies? IFoo \"foo\")" {:classes {'js goog/global :allow :all}})
false

@lilactown
Copy link
Contributor Author

@borkdude I think that's due to another bug - that satisfies? fails to find protocols that don't have methods. For instance, this works:

cljs.user=> (sci/eval-string "(defprotocol IFoo (foo [_])) (extend-type string IFoo (foo [_])) (satisfies? IFoo \"foo\") {:classes {'js goog/global :allow :all}})

And just to show that this isn't a regression, testing js/String using my originally installed version of nbb

Welcome to nbb v0.6.129!
user=> (defprotocol IFoo)
IFoo
user=> (extend-type js/String IFoo)
{:methods #{}, :name user/IFoo, :ns #object[Eq user], :sigs {}, :var #'user/IFoo, :satisfies #{function String() { [native code] }}}
user=> (satisfies? IFoo "foo")
false

@lilactown
Copy link
Contributor Author

Created #785 to track the bug with satisfies? + no methods

@borkdude
Copy link
Collaborator

Thanks!

@borkdude borkdude merged commit 3e068bb into babashka:master Jul 27, 2022
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.

cljs built in types
2 participants