Bluesky #68
Bluesky #68
Conversation
|
Hi @JoDoM , thank you so much for your time on this! I have introduced redux recently but see no problem in removing it in favor of most recent React functionalities (also discussed with @wagnerjt ) (and yes, that's where I had to (cowardly) comment tests :-( ) I'll take a deeper look at the code and let you know if I have any concern. |
|
Waiting for feedback on this change proposal. One thing I could do this coming weekend is split out the code into smaller pieces, starting with removal of redux + package bump, then code restructure and tests |
|
Sorry I've been away for too long! I will review these changes tonight! |
| } | ||
|
|
||
| return serviceInstance; | ||
| })(); |
|
@JoDoM -- Everything looks great to me! You have done a number of todo items for me as well as cleaning up some state/props from child components 👏. I like the singleton neo4jservice you made as well as stubbing out some tests via jest. Good work! One thing we will run into with neo4j 4.0+, is the multi db per session. Neomap currently only supports the default @stellasia -- let me know if you want to review any part of this with you |
|
Sounds good. I'll ping with PRs over the next couple of days 👍🏿 |
This is a blue-sky PR that touches on a bunch of open issues. I looked at tackling just the commented out tests, quickly realizing that the current redux dependency causes a lot of pain to that end.
In general this PR ended up being a clean up of the codebase, partially fixing issues, and hopefully moving repo to a state from which further issues can be worked on independently with minimal cross blocks.
Change Areas
- Code review #12 : Several antipatterns used, example, blocking first render to initialize drivers, using redux store to manage global state for layers, while having layers maintain their own state, mapping props to state, component lifecycle management (easily fixed by moving to functional components). All these can be improved upon with future iterations. Some fixes are proposed in PR
- Upgrade deps; include React Hooks #55 : Upgrade dependencies. Migrate the lightweight components to hooks, and flatten the component tree by merging merging SideBar and LayersList
- Fix tests and CI #62 : Reconsidered the tests and testing infrastructure, proposing Jest test runner and React Testing Library. Jest gives an advantage of being able to run non-react / non-ui tests so we can use the same runner to test services and other utils. React testing lib has an interesting philosophy of testing user actions and interactivity, rather than component state.
Tests introduced don't provide adequate coverage. This PR mostly introduces the new testing structure.
CI loop was not investigated.
PS: PR is not meant for check in as is. Let me know your thoughts on the proposed changes, thereafter we can consider breaking it into atomic changes for check in