fix(setQueryData). When there is no previous instance of the query it tries to create a new one but the type signature was incorrect

[franleplant]

The 2.x version of this reported bug #639

While I was trying to reproduce the reported bug in the 2.x branch I realized that the bug was not present
due to a incorrect function call to buildQuery in the problematic code (problematic respect to the code).

I fixed that and left the queryFn to be undefined to avoid going into the problem condition as explained in the bug.
had to fix tests too.

Bonus:

• have you considered slowly migrating to Typescript? this sort of bug will never happen in typescript (although is minor) and I know from experience that the dev experience is greatly enhanced.
• is there any particular reason you have defined the makeQuery as a function instead of as a class?
• is there any particular reason queryCache is modeled as an object with methods and attributes instead of a class and an instance? The both seem like central "entities" in the library that are instantiated more than once, so they fit the class/instance model pretty well, and with the new syntax they became pretty powerful.

The last two seem like a good use case for classes that could improve the code a little bit.

Respectfully
Fran

[tannerlinsley]

I’ve considered a small bit of TS. I’m not ready to pull the trigger yet though. As for the classes, I know that’s a possibility, but I just prefer the object based pattern. What advantages are you referring to?

[franleplant]

So the benefit I see is structural and syntactic/ergonomic. And I totally see your point of using functions and regular objects, in fact your approach is very similar to what Douglas Crockford proposes in "Javascript: the good parts", in fact what we are trying to represent is data+logic, there are a variety of ways of handling this and all of them have their pros and cons and I completely agree that a lot of it boils down to personal preference and style.

structural

Although making them classes won't necessarily force us to move them to their own file, it probably will tenderly carry us over there. As I said, the Query and QueryCache are super important and fundamental types in this lib so having a class for them makes sense (BTW I am not a class/oop fundamentalist at all, but I do see the value in certain situations). Also, having the two in two different files seems a good way to break them apart as individual units of logic/data abstraction (more on this later). Query.js and QueryCache.js is a super boring way of structuring them (boring is good :) ). Also, breaking them apart will improve the interface between the two.

Interface. Right now is difficult to understand what variables/data/state are part of the "instance" of say a Query and which of them are not (let's say they are modules, helper functions, etc etc like queryReducer), and this boils down to the usage of closures.

Instead of letting queries call this.notiftyGlobalListeners() you are calling notifyGlobalListeners(), and this func is actually a sort of private function of the queryCache instance that instantiated the query that is available through the closure. In the current code you need to go to the definition of notifyGlobalListeners to understand all this because at the call site you cannot be sure if it is a private method of Query or a func defined in another file or something contained in the QueryCache instance (closure).
Compare to this.notifyGlobalListeners where you know that it is just an instance variable/method, and if you want to understand a bit more you just would go to the constructor to understand that you received it as a constructor argument and then you just called it. In class semantics this would be simply an argument that you stored as an instance method.

Syntax/Ergonomics

Interface part 2. Right now we are using a combination of for example public methods like queryCache.getQueryData and pseudo private methods like notifyGlobalListeners (I say pseudo private because although you don't expose it to the outside you are making it available through a closure to the queries you create). Classes have a more clear semantic to express this, and if you use typescript they are event better (IMO) because they are enforced at compile time.

Semantics. Classes have a bit more clear (and boring) semantics for public and private instance variables and methods and they also have a clear semantic for instantiating i.e. makeQuery(args) vs new Query(args) (if you worry about forgetting the new part then there are ways of going around it, but the best is Typescript that will let you know statically that you forgot).

With a lot of this combined together you get IMO

• better interfaces
• better separation of concerns
• better semantics that are old and boring which is good

A lot of this could be achieved with your current style but would require more discipline and conventions.

Enough rumbling. Thanks a lot for taking the time to hear me out. I appreciate you and your work a lot. Thank you.

Fully respectfully
Fran

[tannerlinsley]

🎉 This PR is included in version 2.4.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[tannerlinsley]

All valid points for sure. I think after going through this I would like to at least move all of the different concepts to their own files (queryCache.js, query.js, queryInstance.js) like you suggested. That's at least a step in the right direction. I would still like to keep the object-builder style around for now and keep an open mind to gradually and naturally make some of these changes in the future.

[franleplant]

sure, that totally makes sense. Thinking out loud, it seems that you'd only have `query.js` and `queryCache.js` (in the core module), a query instance is just that, an object that comes out of the `query.js` factory.

…

On Sun, Jun 28, 2020 at 5:24 PM Tanner Linsley ***@***.***> wrote: All valid points for sure. I think after going through this I would like to at least move all of the different concepts to their own files (queryCache.js, query.js, queryInstance.js). That's at least a step in the right direction. I would still like to keep the object-builder style around for now and keep an open mind to gradually and naturally make some of these changes in the future. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#644 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOEIJZNZMSW74EZXMETQN3RY6RI3ANCNFSM4OKGUJCQ> .