Shopify / seafoam-desktop

MIT License
3 stars 5 forks source link

Clean up #41

Closed nirvdrum closed 3 years ago

nirvdrum commented 3 years ago

This PR restructures what owns bits of data. I liked the old RootFolder class from a software engineering perspective, but it unfortunately didn't play terribly well with React. React will re-render if a prop or state value changes, but that generally means a change in object identity (strings are a special case where it's done by value). Mutating the RootFolder instance wouldn't trigger a component re-render on its own, so we either needed to recreate the whole RootFolder instance with new data or use some other mechanism to trigger a re-render.

To help with the mutability, this PR pulls in the immutable module to give us an immutable Map. By allocating a new Map object upon mutation, we can safely use the Map object as a state value and subsequently re-render a component hierarchy. That Map is now owned by DumpFolderTabs as a simple state field. From there, the list of BGV files is passed down via props.

The other big data management issue is tracking the file selected from the dump list. This value is needed by BgvFileList (so it knows which one to render as selected), RightPanel (so the graph can be rendered), and phase selection box (so the phase list can be fetched). Since these components are parts of different hierarchies, I've pulled the state value out to a context provider. Trying to pass the selected file around via props meant the selection event needed to make its way up to a common ancestor and then passed back down as props. With a context provider, each interested component can fetch the value from the context provider and React will re-render any such component should the value change. The downside is if we want to support multiple selected files, we'll need to revisit the context object.

Mixed in with all of this was some new type declarations hoping to clarify some value that were previously typed as string. I don't love what's there right now, but I think the PR is incrementally better than what we had before. I find the difference between filenames and file paths a bit too confusing now. We want just the name for the list display, but we need the full path in order to make any seafoam calls.

Please let me know if you need clarification on any of the changes being proposed here.

nirvdrum commented 3 years ago

Thanks for the thorough review. I have no problem changing that back from a context provider as the application evolves. I initially was looking to minimize unnecessary rendering, but I think that was just making the hierarchy a bit too complicated to work with while we're still actively exploring. At the moment, the selected dump file is used in a fair bit of the UI and handling a state value in the great-grandparent was just getting hard to keep track of. My initial hope was to add a new custom hook (e.g., useSelectedDumpFile) with useState, but the values actually need to be shared. Anyway, it's an excellent note and I appreciate you taking a deep enough look to evaluate that change.

My best guess is a better solution will become clearer with the addition of the phase selector and optionally a tabbed interface for the right panel area.