search

thegazelle-ad / gazelle-server

Server for front-end and editor tools of The Gazelle
MIT License
19 stars 8 forks source link

Hotfix Broken Checker from Falcor 2 Update #371

Closed Helw150 closed 6 years ago

Helw150 commented 6 years ago

Fixes a bug introduced by I believe https://github.com/thegazelle-ad/gazelle-server/pull/361

Falcor doesn't seem to give the $type: "atom" to null anymore which causes pathSetInCache to error out if a value has been updated to null.

emilgoldsmith commented 6 years ago

Also, I hate that you had to go into this code, so sad, sorry haha. One of the things I can't wait to get rid of when we get rid of Falcor Controller, this was the whole reimplementing Falcor part <.<

Helw150 commented 6 years ago

@emilgoldsmith I just threw in the hotfix to the bugs this was causing on my slug PR, not sure if this fixes all bugs really as I don't really want to invest the time figuring out what the whole pathInCache function does and I know this solves the bugs I was facing.

I just threw up the hotfix PR as it didn't make sense to have this on edit-slug (I found this because for about an hour I thought I had introduced the bugs I was getting with slug editing haha) ! I'm just going to pull this branch into my edit slug for now so I can keep making progress on that, but feel free to take this branch over with a better solution.

Helw150 commented 6 years ago

@emilgoldsmith I'm leaving this for now as I want to finish up changes on edit-slug and as I don't feel so bad leaving a bug that I didn't introduce 😝

emilgoldsmith commented 6 years ago

Yeah no worries

emilgoldsmith commented 6 years ago

@Helw150 I looked at it, I was actually surprised that function was still in the code, since we've skipped the tests and I was of the idea that we had removed that function because it wasn't working properly lol :flushed:

I simply removed it now as in the end it's actually a redundant attempt at optimization as if the pathsets are in cache falcor returns as soon as the event loop gives it control, it just isn't strictly synchronous. It came from Ling and I not understanding Falcor properly in the start, whoops.

I also added a test to check that in case that your theory is correct that Falcor v2 cache doesn't wrap null in an atom anymore that the expandCache function handles it (which it seems to do).

It would be great if you could try merging this function into yours and check that it fixes your problems, in that case we can merge in the fix. I'll wait for your approval before merging it

Helw150 commented 6 years ago

Yup works! Thanks for fixing this @emilgoldsmith :)