search

chairemobilite / transition

Transition is a modern new approach to transit planning. It's a web application to model, simulate and plan public transit and alternative transportation.
http://transition.city
MIT License
20 stars 13 forks source link

Don't recalculate transferable nodes on startup #930

Closed greenscientist closed 2 months ago

greenscientist commented 2 months ago

We now save the transferable nodes in the database, so we don't really need to recalculate them at each startup. But we still need to save them to the capnp cache file.

We add a saveAllNodesToCache function to NodeCollectionUtils, based on saveAndUpdateAllNodes, that only store the node data without recalculating it. We add a else in the recreateCache function where we test for the refreshTransferrableNodes flag to call our new function in that case. (That way we always save the transferable nodes to cache)

The dbToCache were missing a mockClear, we switch it to jest.clearAllMocks() and fixes the erroneous tests

The call in socketServerApp was changed to ask for not refreshing the transferable node when recreating the cache on startup

greenscientist commented 2 months ago

This save 5 minutes on startup on my machine. (on a total that was about 7 minutes)

tahini commented 2 months ago

Please run yarn format to get the linter to pass

tahini commented 2 months ago

Also, isn't there an issue related to this PR? If so, please add to the commit message.

greenscientist commented 2 months ago

Also, isn't there an issue related to this PR? If so, please add to the commit message.

It did not add it because it did not want for github to close the issue. It's not fully solving it (#898)

greenscientist commented 2 months ago

Set to draft. Something in my test, we are missing some data in the cache files.

greenscientist commented 2 months ago

Added the ugly quick fix

greenscientist commented 2 months ago

Le test fail, mais c'est. Ça fonctionne avec trRouting?

I know... I don't know why the test for the previous function have functionned at all :(

Mais oui, ca fonctionne avec trRouting.

greenscientist commented 2 months ago

Yeah, no I really don't want to add more test for that. I don't see any way to do a clean test for that. It should all go away anyway with a good refactor. This data structure really upset me with the way it is at the moment!

tahini commented 2 months ago

What upsets you so much? Transferable nodes are a concept outside the node itself, they should not be inside the node. Is it because it is an optional property of the node's data field that bugs you most?

greenscientist commented 2 months ago

What upsets you so much? Transferable nodes are a concept outside the node itself, they should not be inside the node. Is it because it is an optional property of the node's data field that bugs you most?

You say it's separate, by everywhere in our applications, it's contained inside the Node (in TrRouting, Transition, in the capnp cache). If it's a separate thing, it should be it's own object, not part of a Node one.