Fork me on GitHub
#fulcro
<
2020-08-18
>
Tyler Nisonoff00:08:13

i could make a small repro repo if you’re interested, but assuming you don’t have the time to go through all that. Could be a good exercise for me to just verify its not something else funny going on in my app

tony.kay00:08:38

not much time here…but could just be a bug of how I’ve done the search alg for the RAD routing helpers. You could make an alt form for standalone with diff name/prefix and add that to router as workaroudn

👍 3
nivekuil02:08:05

there's a line that throws a warning from shadow-cljs in legacy-ui-routers.cljc: #?(:cljs (set! js/React react)) I removed it in my own code and stuff seems to work ok. PR wanted?

------ WARNING #2 -  -----------------------------------------------------------  Resource: com/fulcrologic/fulcro/routing/legacy_ui_routers.cljc:2:1  constant React assigned a value more than once. Original definition at externs.shadow.js:4 --------------------------------------------------------------------------------

tony.kay02:08:15

so, @kevin842, technically shadow-cljs makes it so we can alias React out of the non-global space, but for legacy reasons some ppl might still use cljsjs, so if I write Fulcro to only assume shadow, it may fail for some users that only use cljsjs if I don’t rely on a global sym…but shadow won’t add that global sym. I also don’t know what order things will load in…so, kind of a mess. If anything, at this point, I should probably assume people have outgrown cljsjs and just make a release that cuts and runs from it, but then I have to test to make sure what I do works with both standard cljs compiler and the shadow-cljs build tool…which is a lot of testing I don’t have time for. If you are interested in putting together a version of the template that uses non-shadow tooling so both tool chains can be tested, then I’d love to publish that alongside the current template (could even just be an alt branch) and ditch the ugliness

tony.kay02:08:52

so, those lines are me currently being paranoid since I have not tested the “std” tool chain

tony.kay02:08:58

and there could be ordering issues that result from such side-effecting garbage…it needs a good cleaning, I just don’t have the time. I will welcome a PR for a template branch with non-shadow compile and a companion PR that cleans this up, but otherwise it can just sit for nwo

tony.kay02:08:05

really, I was halfway through ditching cljsjs, and I realized I could possibly be breaking things…and I don’t want to even imply that I’ve left anything very sane sitting around…no one has complained, till now 😜

nivekuil02:08:33

since this warning only appears if I load legacy￱￱ui￱￱routers.cljc, if the line in question is necessary for avoiding breakage in other toolchains, then it's already the case that most of fulcro3 is incompatible with them right?

tony.kay02:08:52

I’ve peppered that line about in a couple of places

nivekuil02:08:01

huh, ripgrep only shows me one line for set.￱*￱js/React. in any case thanks for the context, I'll leave it be for now -- still getting to know fulcro. Lots of fun so far :)

tony.kay02:08:40

hm…perhaps I changed my mind at some pt and forgot to remove that one…entirely possible

tony.kay02:08:44

too many things 😄

tony.kay02:08:57

that rings some kind of bell, actually

thheller08:08:00

@tony.kay FWIW unless you want to support old versions of CLJS and CLJSJS you can do (:require ["react" :as react]) without the global just fine nowadays even outside shadow-cljs

👍 3
thheller08:08:11

cljsjs/react has provided the necessary :foreign-libs stuff to expose that name for a while now and moving away from the js/React global will make things much easier to people using the :target :bundle from CLJS

thheller08:08:19

but yeah would probably still require some kind of test suite 😛

Adam Helins12:08:43

Just so I know if it is worth the trouble, when checking coherence between props destructuring and the component's query, Fulcro does not seem to checking beyond one level of nesting, does it? For instance, :a within :prop

[{{:keys [a]} :prop}] 

cjmurphy14:08:21

Depends on the render you use. keyframe-render2 will allow you to see props that perhaps in some perfect world should be the props for another component that your component only knows about by get-query.

tony.kay14:08:48

I think he’s talking about the error checking the macro does on the props destructuring/query/initial-state/ident

Adam Helins20:08:32

Indeed I was, thanks

xceno15:08:23

I'm currently testing https://github.com/fulcrologic/fulcro-rad-datomic/pull/9 for fulcro-rad-datomic, with the fulcro-rad-demo but it throws an exception https://github.com/tylernisonoff/fulcro-rad-datomic/blob/develop/src/main/com/fulcrologic/rad/database_adapters/datomic.clj#L533 :db.error/unsupported-protocol Unsupported protocol :dev And I assume, that's because the code uses the peer-api and not the client-api. Could that be correct? (I know this is more than bleeding edge right now, sorry)

tony.kay17:08:42

Thanks for testing… @U016TR1B18E will have to chime in on that

Tyler Nisonoff19:08:38

Yes, the demo is written for on-prem, so you need to replace requires of the form [datomic.api :as d] to [datomic.client.api :as d] I can try to throw together a demo fork that uses the PR this week — its pretty simple beyond that. The other change I can think of off the top of my head is in src/config/defaults.edn I set something like this:

:com.fulcrologic.rad.database-adapters.datomic/databases
                                    {:main {:datomic/schema :production
                                            :datomic/database "example"
                                            :datomic/client {:server-type :dev-local
                                                             :system "dev"}
                                            :datomic/prevent-changes? true}}

Tyler Nisonoff19:08:40

@U012ADU90SW happy to answer any questions of issues you run into — threading here is probably best so others can see them if they try it out soon. I’m hoping to touch up the PR tonight / tomorrow

xceno20:08:51

Thanks guys I really appreciate your efforts! I monkey patched the require statement some hours ago, but probably messed something up, because I got another error. I'll try a fresh checkout from source for everything and give it another shot later. Here's my config btw. I just tried to throw a quick test together based on the official datomic dev-local setup:

:com.fulcrologic.rad.database-adapters.datomic/databases
                                    {:main {:datomic/schema           :production
                                            :datomic/driver           :dev
                                            :datomic/database         "example"
                                            :dev/host "localhost"
                                            :dev/port 4334
                                            :datomic/access-key       "myaccesskey"
                                            :datomic/secret           "mysecret"
                                            :datomic/prevent-changes? false}}

Tyler Nisonoff20:08:21

oh the queries in database_queries.clj use on-prem only syntax — I had to re-write the queries slightly to work for cloud

Tyler Nisonoff20:08:45

I should throw up a version of the demo that works with the PR — can try to get to that later tonight / tomorrow

Tyler Nisonoff20:08:26

/ maybe I’ll submit a change to the demo that uses the client query syntax (that on-prem also supports)

👍 6
Tyler Nisonoff02:08:58

ugh, the peer and client APIs are too different to unify the queries… the return types from d/q are different 😞

xceno07:08:46

yeah, I spent the night replacing peer with client API calls but as you found out too - that's a pretty bad idea 🙃 I had another demo project (luminus based though) lying around. Turns out you can leave the peer-api as-is but you need to change the dependency from datomic-free to datomic-pro . This way I got my dev-local setup to work on that luminus demo

tony.kay15:08:30

There is middle ground in the APIs…you could just rewrite the functions so they work right in both? Or are you saying that would just add in weird conditionals?

Tyler Nisonoff16:08:22

It would add weird conditionals — I’m sure I can do it, was just making the code a little gnarly i can write some helpers and use those and get something working, just wasn’t sure if you’d want that complexity in the demo, or if it’d be worth just maintaining a separate rad cloud reference repo somewhere (I could host a fork and we could point to it from the README, for example) I can take another stab

tony.kay17:08:09

I don’t use client API, but my impression was that it just “had less”: subset of main API: no entity API, more limited q, and no pull-many.

tony.kay17:08:59

In that case, why not rewrite the functions to “do the right thing” with the subset API that works the same in both. That would make the demo minimally complex, since otherwise we’d have to add yet another source path alias to swap out files.

Tyler Nisonoff17:08:19

thats what i thought as well, but using the same API subset, i get different return formats, so its not that simple 😞

tony.kay17:08:00

well, given that you have to have two diff requires, I guess we already needed source magic

Tyler Nisonoff17:08:25

yeah, so i think there are three options: 1. Write some helpers / add some complexity to database_queries so that you just swap in the right require in the demo and it works 2. Do nothing, someone else sets up an example demo 3. set up a datomic_cloud directory and alias, which duplicates database_queries, but automatically works for cloud

tony.kay18:08:12

4. Make a branch where the datomic code is for cloud

Tyler Nisonoff18:08:07

happy to do whichever you prefer -- want me to start with (4)?

Tyler Nisonoff01:08:38

PR here: https://github.com/fulcrologic/fulcro-rad-demo/pull/13 I need a cloud branch set up in the main repo I can merge this into — then we’re good to go

tony.kay14:08:57

branch created and pushed

tony.kay14:08:14

on testing: I just need to know that using the new rad datomic adapter with the demo in SQL mode works, and using it in regular Datomic mode works…i.e. pre-existing behavior is not broken.

tony.kay14:08:26

of course, it is also good if the cloud stuff works 😄

tony.kay14:08:04

also is there a doc update in readme? @U016TR1B18E

Tyler Nisonoff16:08:35

I’ll update the REAME Regular Datomic works if we add the dev local package to the demo, will get that PR out too I forgot to test SQL, but can do that too

xceno12:08:20

Thanks for your work guys! I'm going to test the datomic-cloud PR now and I can also try SQL with a local postgres db later

xceno15:08:48

Oh I forgot to mention this, I'm testing the PR with a datomic dev-local setup