Fork me on GitHub
#fulcro
<
2017-11-01
>
currentoor01:11:14

@tony.kay since you say it does not make sense for a component to not have an ident, does that mean we should only use functions (instead of sub-components) when we want to extract logic out of a component?

currentoor01:11:58

Also are you saying it doesn’t make sense for a component to not have an ident whenever you call transact! on it? Or just in general all components should have an ident?

tony.kay06:11:07

@currentoor I did mean stateful components (with a query)…not just plain-jane React components

claudiu06:11:44

@tony.kay Sounds really awesome. Since it sounds like a full re-write, any chance some changes will go in to make df/load, df/load-action work on the server for SSR ?

claudiu07:11:48

The current process is a bit manual. Keep thinking about how nice it would be when creating the application on the server, to just give it the fulcro-server-parser. All requests just go to the multimethods instead of http calls (no need for batching).

baptiste-from-paris15:11:21

hello friends, I was looking at the fulcro-template project

baptiste-from-paris15:11:43

I noticed that the error message when you’re trying to login with bad credentials never appears

baptiste-from-paris15:11:31

Looked at the app state and indeed, the :current-user is nil even if attempt-login returns it

baptiste-from-paris15:11:47

tried to solve this one but did not find out

baptiste-from-paris15:11:55

am I the only one with this bug ?

baptiste-from-paris15:11:45

and offcourse /api returns data so it’s more a merge/query bug

baptiste-from-paris15:11:49

["^ ","~:current-user",["^ ","~:login/error","Invalid credentials"]]

tony.kay15:11:24

@claudiu It isn’t a full rewrite at all. It is mostly done. It is a few internal changes to the Om Next internals that are not in line with Om Next’s design. The fact that we choose the default db format as the only db format are what enables them to be easily written.

tony.kay15:11:52

But, I hear you on the load helpers for SSR. The specific thing you’re asking for is probably not easy, but something roughly equivalent and nice is possible

tony.kay15:11:11

@baptiste-from-paris Hm. Yeah, I didn’t debug that bit of the template much…was just trying to show structure

tony.kay15:11:40

I doubt it is a bug in Fulcro, since that it such a core simple thing…

baptiste-from-paris16:11:23

yes offcourse, as I am learing fulcro, I don’t know where to look appart from app-state, query (have already some om project in prod)

baptiste-from-paris16:11:38

but could it be a merge problem ?

tony.kay16:11:56

Yes, it could

tony.kay16:11:00

Let me look at it with you

baptiste-from-paris16:11:27

I have a meeting right know, but I’ll look at it again in 2/3 hours after work

tony.kay16:11:38

k. I’ll let you know what I find

baptiste-from-paris16:11:44

great, thanks a lot

fatihict16:11:17

@tony.kay Why is Fulcro 2.0 not called Fulcro Next? 😄

tony.kay16:11:05

cause it isn’t changing from an external viewer’s perspective 😉

tony.kay16:11:18

same API, same structure…just no dependency on Om Next

tony.kay16:11:52

and no broken dynamic queries with respect to history, time travel, and support viewer

tony.kay16:11:22

@baptiste-from-paris So, the error message from the server needs to have a :uid for it to normalize…that’s why it is failing

tony.kay16:11:00

Unfortunately, the UI code isn’t written to handle the case of a user whose login failed, so when I “fix it”, it lets you log in with any credential 😉

tony.kay16:11:41

I’ll patch it and push a new version on develop

baptiste-from-paris16:11:51

would never have found this one

tony.kay16:11:23

yep. Just think about how normalization works…that’s the only thing to know

tony.kay16:11:31

it calls the ident function from the component on the props.

tony.kay16:11:44

if the props don’t have sufficient data: no good

tony.kay16:11:16

it’s basically just tree->db with a deep merge

tony.kay16:11:05

Fulcro adds in some other bits…one other problem was I didn’t include the :login/error in the User query…so that couldn’t go in state either…so you end up with trying to merge nothing at nowhere

tony.kay16:11:28

and tree->db wipes those out

baptiste-from-paris16:11:48

but the uid would have been hard to solve ^^

tony.kay16:11:06

if you’d fixed the query, it would have merged/normalized at ID nil

tony.kay16:11:14

so it would have shown up in a way that led you down the path

tony.kay16:11:45

the only reason it got completely wiped out was that the map ended up empty because there was nothing that matched the query

baptiste-from-paris16:11:37

nope it did not but I might have made it wrong, will look after this stupid meeting 😉

tony.kay16:11:53

I’ll double check my assertion there

baptiste-from-paris16:11:33

one thing that I noticed (also about the template) is the lack of href on anchor elements

baptiste-from-paris16:11:40

good for SEO on SSR

tony.kay16:11:41

you’re right…if it can’t normalize, it does seem to wipe it out

wilkerlucio16:11:15

@tony.kay maybe Fulcro 2.0 is a good place to upgrade React to 16? what you think?

tony.kay16:11:18

I’m not too worried about SEO on the template…you’re just going to completely change the UI anyway is my tought

tony.kay16:11:33

@wilkerlucio yes. There is already a patch to do that on Om Next itself. I plan to pull it over

wilkerlucio16:11:53

yeah, had you tried it? I didn't had the chance yet

wilkerlucio16:11:13

last I remember Antonio was fixing adv compilation issues

baptiste-from-paris16:11:21

it has been merged

wilkerlucio16:11:24

not sure if that was completed

tony.kay16:11:54

Easy enough to test out. I’ll wait to do that till I’m done with the other stuff

wilkerlucio16:11:08

sure, makes sense, I'm excited for Fulcro 2.0 🙂

wilkerlucio16:11:14

and React 16, hehe

wilkerlucio16:11:43

one thing that we should test is about the new possibility of retuning lists out of renders

wilkerlucio16:11:24

probably will need a bit of conversion on the edges

tony.kay16:11:53

lists out of renders? Oh, you mean a vector of divs, etc?

tony.kay16:11:17

seems tractable

tony.kay16:11:52

would it break React using Fulcro components? E.g. If I define a component in Fulcro (without queries) and export it to js-land, can I use it if it returns a vector of things?

tony.kay16:11:14

That is an important core requirement. cross-functionality with React

tony.kay16:11:19

not just one-way

baptiste-from-paris16:11:54

ok, could try it with uid and it’s workgin

wilkerlucio16:11:05

yeah, the React 16 allows renders to return lists, so they can output multiple elements

baptiste-from-paris16:11:17

Which is indeed logical because it’s waiting for a user

wilkerlucio16:11:17

I think would be easy, just convert to JS array on the edge of the factory

baptiste-from-paris16:11:07

I don’t know what’s the typical way to handle errors yet but I guess one way would be to have a union query to handles errors. As an error is not a User

claudiu19:11:48

@baptiste-from-paris for login I ended up just using a query, just df/load and in the post mutation I make the decision the response returns a error or a valid user :)

baptiste-from-paris19:11:28

ok, thanks, nice 🙂

tony.kay16:11:16

@baptiste-from-paris On this front I’m not going to be that picky….You got back a non-existing User

tony.kay16:11:22

that works for my mind

tony.kay16:11:30

You asked for a user…you got a “bad one”

tony.kay16:11:40

rename the :login/error field if it bothers you 🙂

baptiste-from-paris16:11:53

yes, it’s acceptable, you’re right

tony.kay16:11:00

[:user/id :user/name :user/name :user/why-i-suck?]

baptiste-from-paris16:11:28

I guess there is the same problem with :server-down

baptiste-from-paris16:11:35

have to check also

baptiste-from-paris16:11:37

and I’d love to add some dynamic routing example to the project

tony.kay16:11:45

So, that is part of the plan with 2.0…to give better abilities to deal with temporary outages. With working history nav and idempotent mutations we can retry operations automatically in this common case, and not have to write code at all (other than a single global “this has gone on too long” kind of way)

baptiste-from-paris16:11:00

I need that for my project so I might have the time to add that if you’re okay

tony.kay16:11:42

need which? server down? At the moment there is a hook for global network errors. I recommend just a modal overlay.

tony.kay16:11:02

if you can be more specific I can help more…not sure what you’re asking to add?

tony.kay16:11:19

oh, you mean to the template?

baptiste-from-paris16:11:26

No, adding an example of dynamic routes to the template

tony.kay16:11:48

dynamic routes? You mean with dynamic router and code splitting?

tony.kay16:11:04

yeah, I missed that one message, sorry

tony.kay16:11:49

So, Dynamic Router is broken at the moment with respect to history. So, until 2.0 I don’t consider it a solid feature. However, the API won’t change. Not sure I want to add that much to the template. It is already quite large, and that complicates the build, etc.

tony.kay16:11:13

I’d recommend you fork the template and publish it. Glad to add a link to it from docs

tony.kay16:11:55

I want to keep what I inherit to maintain to a manageable level 🙂 It’s already a lot

tony.kay16:11:03

So, I think I just found a bug in Om Next…has anyone noticed that setting React component local state on a component that has a query re-runs the query against the parser?

tony.kay16:11:32

@gardnervickers, I know you have performance concerns with your UI…have you seen this?

tony.kay16:11:00

@mitchelkuijpers You just did DnD with setState, right? Did you notice the queries running for that component?

gardnervickers16:11:38

@tony.kay we don’t use component local state.

tony.kay16:11:32

Oh? Really? You get that UI performance with only using transact???

gardnervickers16:11:24

We do some things out of the regular transact loop using a :shared and merge-state to get performance.

tony.kay16:11:52

but that swaps against the atom and causes a full UI refresh…I guess that’s faster than hitting db->tree and path-meta. Interesting. That confirms a suspicion I’ve had about larger apps: that the general optimization of shouldComponentUpdate is already quite good

tony.kay16:11:43

With path-opt working correctly (Fulcro 2.0 fixes as well), you may find no need for your current tricks 🙂

gardnervickers16:11:28

Nice, we’re actually using pathopt in production, we have a shallow enough render tree we don’t hit any problems with it.

gardnervickers16:11:38

om/merge! causes a full query?

tony.kay16:11:16

If you hit the state atom, there is a watch on it that schedules a render. That kind of render will run a full query

tony.kay16:11:29

(unless something has been added to queues)

tony.kay16:11:41

let me look at merge…

tony.kay16:11:44

I don’t remember 😉

tony.kay16:11:09

om/merge! queues keys, so it will do targeted queries and renders

tony.kay16:11:55

Shared is another problem…it isn’t in history anywhere either

tony.kay16:11:09

I won’t break it…just saying it is currently broken with respect to history nav

tony.kay16:11:23

I intend to fix the history part

gardnervickers16:11:52

We don’t mutate it, we initialize it on app mount so it’s working with our history viewer.

tony.kay16:11:19

Oh, I see…yeah, but it can change over time, and there is no recording of it.

tony.kay17:11:41

@wilkerlucio on React 16. You had the final suggestion for create class in Om issue 881. Is that all that remains?

gardnervickers17:11:29

@wilkerlucio It took me about an hour to convert our app over to pathom last night and we were able to ditch about half of our server-side parsing logic plus replace our inconsistent version of ::p/entity and ::p/path which is fantastic. Really nice work!

wilkerlucio17:11:14

@tony.kay I think was superseed by the changes Antonio did

wilkerlucio17:11:50

@gardnervickers thanks for letting me know 🙂

wilkerlucio17:11:31

are you driving a database directly, or connecting services?

wilkerlucio17:11:49

@tony.kay just saw the issue about shared, you said it's not on history, do you think it should be part of the state?

tony.kay17:11:37

that’s my first inclination…I’m need to think through the use-cases

tony.kay17:11:58

I mean, it allows a function at the moment, so it can evolve over time, but nothing is snap-shotting what it was in the past

tony.kay17:11:23

I’m also wanting to add the tx that caused the history state progression, so it might make more sense there

wilkerlucio17:11:27

the cases I used shared were mostly to inject stateful shared stuff

wilkerlucio17:11:46

so this worries me, if they get on the DB it will break things

tony.kay17:11:48

ooof…like js mutables?

wilkerlucio17:11:59

like core-async channels, and other mutable stuff

wilkerlucio17:11:15

when I want data, I just throw it at the app state

wilkerlucio17:11:43

for what use cases are you thinking for shared?

tony.kay17:11:08

I’m just saying, if you want to navigate to history point 5 steps ago, shared breaks it the way you’re using it

tony.kay17:11:33

I mean, it is broken in any case at the moment

tony.kay17:11:45

(except for sharing constants…but why not just make those global?)

wilkerlucio17:11:57

because I want contextualize by the app

wilkerlucio17:11:03

I might have multiple fulcro apps running

tony.kay17:11:21

yeah, I understand that…though you can get that using adv compiler option :ouput-wrapper

wilkerlucio17:11:43

I just don't see why throw data on shared, if you can put it on the app state

tony.kay17:11:55

I agree with that sentiment

wilkerlucio17:11:06

maybe a wrong perception, but I always though shared as a place for stuff that I can't put on the app state

wilkerlucio17:11:53

so, IMO should be a dev concern to ensure bad state on shared doesn't break the app somehow, not the FW

tony.kay17:11:40

I always thought of shared as a place to put stuff that most components needed, but for which you didn’t want to write a link query

tony.kay17:11:59

like current user, or locale

wilkerlucio17:11:27

if you go that way, where would I throw those shared stateful things? that's my worry

tony.kay17:11:42

I’m ok if you want to do something that is incompatible with serialized history…your choice. But I do want to make it work if you choose to make it work.

wilkerlucio17:11:07

maybe make the "shared in state" optional

tony.kay17:11:15

Shared stateful things could be global. Use :output-wrapper

wilkerlucio17:11:33

what is :output-wrapper?

tony.kay17:11:52

It tells Google Closure to wrap your entire app in a function, so it doesn’t pollute global

wilkerlucio17:11:21

but that's on app level, I mean, when I was writing my blog to have live examples, in my own source I have multiple instances of fulcro app loaded

tony.kay17:11:32

I have not personally tried it, but ran across it when thinking about the problem of Closure renaming crap in a way that accidentally collides with others, which is quite possible

wilkerlucio17:11:41

the shared is the place I have to put context for each running fulcro client

tony.kay17:11:42

Oh, yeah, I see

wilkerlucio17:11:05

if I move to a global, I would have to start filtering it somehow on my app, and lose the local context

tony.kay17:11:07

ok, so in that use-case, support viewer simply cannot work. stateful breaks it no matter what

wilkerlucio17:11:36

so, if you feel strong about having shared on state, maybe we have a flag to turn that off?

tony.kay17:11:41

Why are you polluting your application with these stateful things outside of the network layer?

wilkerlucio17:11:07

but those shared get on the network layer as well

wilkerlucio17:11:11

I use then there

wilkerlucio17:11:20

sorry, no network, mutations

tony.kay17:11:41

Let me better state what I’m thinking.

wilkerlucio17:11:54

for example, if I want to have a component that stores local data, I can have an implementation using LocalStorage for the browser, and a differnet one for a Chrome Extension

tony.kay17:11:13

The model of Om Next (and Fulcro) is that you have queries and mutations. Nothing else. The mutations are meant to do local optimistic updates and “remoting”. In my mind, “remoting” covers any side-effecting, async thing.

wilkerlucio17:11:18

this is the kind of setup I like to do at client level, and currently the :shared is the only place I can contextualize on that level

tony.kay17:11:53

so, you could write your example as a “remote” that is implemented locally on the client. Now it is just catching that mutation or read

tony.kay17:11:13

pushes all of that out of the primary model space

mitchelkuijpers17:11:20

@tony.kay yes we noticed this too, we used react-set-state

tony.kay17:11:35

@mitchelkuijpers ah, so you went around it 🙂

wilkerlucio17:11:38

but I think writing as a remote is more complicated

tony.kay17:11:58

@wilkerlucio Is it? It might be a little more code, but doesn’t it keep the core model super simple?

tony.kay17:11:09

you have queries. You have mutations.

wilkerlucio17:11:11

my main point is, currently :shared is the only place I can have client-level stuff that doesn't have to be on the app-db

tony.kay17:11:21

not arguing that

tony.kay17:11:33

I’m arguing putting stateful stuff in a place that breaks features of the system

wilkerlucio17:11:43

if we took that way, we lose that possibility

tony.kay17:11:01

please clarify…lose what possibility?

wilkerlucio17:11:16

of having stateful stuff contextualized by client

wilkerlucio17:11:37

I have another suggestion, maybe we have 2 different shared, one that does in the app db, other that doesn't

wilkerlucio17:11:04

I just don't want to lose that option, I don't use it much, but I like knowing that if I need to use the option exists

tony.kay17:11:40

So, I don’t think putting shared in app state is what I’d do either way. Too easy for collision, and I can hang it on the transition edge of history

tony.kay17:11:01

but if you put non-serializable things in it, you lose support viewer.

wilkerlucio17:11:06

ok, if it doesn't polute the app-data, I don't see any problems

tony.kay17:11:10

nothing I can do about that 🙂

wilkerlucio17:11:33

sure, I understand that if I do that, I have to deal with the consequences, and I'm ok with that

tony.kay17:11:11

I would encourage you (and anyone lurking) to consider the opinion that you should try your best to avoid anything in mutations on the client that side-effect beyond state. Using the “remote” mechanism pushes the more complex things (like async, etc) out to the layer that is designed for them. If you don’t care about support viewer and history navigation, then it matters less.

tony.kay17:11:02

Delegating to a “remote” doesn’t mean it has to use the network

tony.kay17:11:28

(load this :boo Boo {:remote :local-storage}) is a perfectly fine abstraction

tony.kay17:11:28

(transact! this [(set-local-value {:v 1})]) with (defmutation set-local-value [{:keys [v]}] (local-storage [env] true))

tony.kay17:11:50

and an initial setup that plugs a “remote” into the client with name :local-storage

tony.kay17:11:51

That thing gets queries and mutations, so you end up just using a parser to handle the requests….so, check out pathom 😄

tony.kay17:11:05

The elegant simplicity of the query/mutation/immutable app state keeps your reasoning about things quite clear…AND having those ugly things at the “network” layer also means history (as I’m defining it) will have a record of why state changed from A to B (e.g. because mutation X or query Y returned)

tony.kay17:11:21

talk about much easier diagnosis of problems!

tony.kay17:11:00

Yet more examples I obviously need to write 😜

tony.kay17:11:35

and also why I want history fixed to work fully!

mitchelkuijpers18:11:26

@tony.kay we use shared for constants so we can also render server side. Then real globals get really annoying

mitchelkuijpers18:11:43

We also have remotes for the HTML5 history and localstorage btw

claudiu08:11:00

@mitchelkuijpers Thanks for sharing. Never thought of remotes for those scenarios, sounds like a really nice & elegant solution.

wilkerlucio18:11:56

@tony.kay sure, there are ways around it, ok, I'll try that, and I'll ping you if I ever get in a place that shared seems reasonable 😉

wilkerlucio18:11:25

and a separated remote to that seems like a good idea

gardnervickers21:11:49

@wilkerlucio Wrt/ your previous question about our use of pathom, we’re doing joins between a bunch of data sources.

gardnervickers21:11:04

It really shines there, especially since Datomic is our main source of truth linking all our foreign keys.

roklenarcic21:11:44

when should I use om/set-state instead of using state in app atom?

tony.kay21:11:18

The intention of component local state is to handle fast-changing transient values, like DnD position and animation status

tony.kay21:11:33

you don’t want the overhead of swapping on the app state, and don’t need that data in history

roklenarcic21:11:17

Ok, so not for determining if something is expanded or in edit mode or things like that?

tony.kay21:11:58

not if you want history to work on it, no

tony.kay21:11:14

reasonable to use for keystrokes in an input field, then move it to app state on blur

roklenarcic21:11:53

If I have a list of people, where I can click on one person, which then has detailed view which has list of transactions this person participates in, then if I click on transaction I get list of persons involved, and when I click on the person I get to same detailed view. How do I accomplish that and avoid problems with recursiveness of queries?

tony.kay21:11:44

Use a different component for the nested one

tony.kay21:11:58

otherwise you’re making infinite recursion.

roklenarcic22:11:24

Ideally user can click on person->transaction->person->transaction ad infinitum, if they wanted to do so

tony.kay22:11:39

well, in that case you need true recursive queries

tony.kay22:11:36

How do you want the UI to look, though. You can’t nest that visually

roklenarcic22:11:51

It keeps swapping screens

roklenarcic22:11:58

not visually nested

tony.kay22:11:34

right, so it isn’t a recursive query…you just change the idents to point to the next iteration, and the “nested” people are a different component that don’t list txes…then you shift state

tony.kay22:11:17

PersonList -> PersonDetail -> Transactions -> PersonSummary

tony.kay22:11:34

clicking on PersonSummary makes them the target of PersonDetail

roklenarcic22:11:34

I understand.

tony.kay22:11:38

So, my tests of ILocalState seem to indicate that it is also buggy in Om Next. I’ve never seen even someone do an example of it (I had to read the source to see how it is supposed to work, and I see a bug as it stands that I can produce in a devcard). I’m kind of guessing no one in Om or Fulcro world is using it…if anyone thinks otherwise, I’d like to hear it. It’s another source of complexity I’m probably going to drop.

tony.kay22:11:06

Also, I’m going to make set-state! be the same as react-set-state. The reasoning, as best I can tell, is to prevent react from updating the UI outside of the render animation loop, but unfortunately there is no way for the render loop to know not to run the query on the component, so IMHO it makes set-state! mostly useless for the high-performance transient tasks I think it should be used for.

roklenarcic22:11:23

Earlier you said you see no reason why every component should not use an ident. What would you use as an ident in a component that represents many-to-many relationship?

roklenarcic22:11:33

Or one to many

tony.kay22:11:42

should not use an ident

tony.kay22:11:55

component data should be normalized or you’re making life difficult for yourself

roklenarcic22:11:59

yes, should not

tony.kay22:11:13

so, every component (that has state) should also have an ident

tony.kay22:11:51

unrelated to relations…they all need a unique place to live

roklenarcic22:11:43

Yeah, but if I have a component that's a badge that shows how much a person spent in a particular transaction, I have basically two datoms: amount and person id. I can't make a good ident out of that

tony.kay22:11:29

then it becomes part of the parent’s data, and you don’t have a component with a query

tony.kay22:11:37

I didn’t say everything had to have a query

roklenarcic22:11:38

neither is unique and even the combination isn't. So I'd have to push down transaction id and have an ident of trn id, person id pair

tony.kay22:11:05

just because you want to split out something with a defui doesn’t mean it has to be a stateful component

roklenarcic22:11:10

so I make a more complex query in parent component and just pass props down to render?

roklenarcic22:11:25

yeah that could work

tony.kay22:11:54

yeah, don’t get over-zealous with the queries…if they don’t make sense for the data-driven aspect (e.g. have a unique identity on the server), then they are not buying you much

tony.kay22:11:17

well, that’s an oversimplification, but hopefully you get it

tony.kay22:11:47

if you need to mutate it as a client-side stand-alone entity, then you want it to have an ident…that is a better simplification

tony.kay22:11:37

So, I’m going back on what I said above. It is technically useful to have set-state! be different than react-set-state!

tony.kay22:11:14

I think…it is a tough one

tony.kay23:11:38

So, the current implementation of set-state! defers rendering until the next animation frame by offloading it to the reconcile stage (at which point queries are run) This prevents over-rendering (lots of set-state! calls causing many DOM updates), but forces the UI refresh to always take the overhead of a full reconcile for all queued components/keys. This makes set-state! a friendlier citizen IF you’re using it for slow state storage, but means you have to revert to react-set-state to get the really performant one. I had not realized this was there, and had always assumed set-state! just wrapped React’s version. My personal philosophy is that set-state should refresh the component right then, but chains of set-states (e.g. child sets state, then does callback to parent that sets state) would then cause two forced refreshes at the actual DOM level. This is another one where I feel like the added complexity of the internals might not be worth the outcome. Usage of set-state! in general is already out-of-bounds for our reasoning model, and really should only ever be used for animation kinds of things and low-level DOM crap that cannot be represented via state (e.g. carrying around refs to components to do image manipulation).

tony.kay23:11:29

I also see a technical issue where if you mixed calls to set-state and react-set-state you could screw things up

claudiu08:11:00

@mitchelkuijpers Thanks for sharing. Never thought of remotes for those scenarios, sounds like a really nice & elegant solution.