Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: store file content in atom #105

Merged
merged 19 commits into from
Jun 9, 2023
Merged

Conversation

sehyod
Copy link
Collaborator

@sehyod sehyod commented Jun 7, 2023

This PR fixes #32

This splits the state in a more atomic way, with the following:

  • core primitive atoms storing simple data
  • core action atoms doing simple operations
  • exported combined atom holding the logic and calling the core atoms

This also adds an activePane state, that enables the app to open new file in the pane that has been used in last.

@sehyod sehyod requested a review from cguedes June 7, 2023 11:32
@sehyod sehyod self-assigned this Jun 7, 2023
@sehyod sehyod mentioned this pull request Jun 8, 2023
4 tasks
Copy link
Collaborator

@danvk danvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see the atoms wired up! Lots of suggestions but at a high level:

  • Now that chore: set up frontend testing infrastructure #100 is in, this really needs some tests. It would be great to have tests for some of the invariants that you're maintaining, for example that a file gets purged from memory once it's closed in both panes. OK if it's in another PR, but it should probably the very next PR!
  • I think we should use an eslint rule to enforce that "core" atoms are only used in the expected places, rather than relying on comments and underscores. See comment below.
  • I believe Jotai's atomFamily is designed to eliminate some of the bookkeeping around Maps.

It's less scary to refactor once you have tests, so maybe a path forward could be to add some unit tests for src/atoms/fileActions.ts (the public state API) and then refactor the internals to use atomFamily.

src/atoms/core/activePaneAtom.ts Outdated Show resolved Hide resolved
src/atoms/core/fileContentAtom.ts Outdated Show resolved Hide resolved
src/atoms/core/fileContentAtom.ts Outdated Show resolved Hide resolved
src/atoms/core/fileContentAtom.ts Outdated Show resolved Hide resolved
src/atoms/core/fileContentAtom.ts Outdated Show resolved Hide resolved
src/panels/MainPanel.tsx Outdated Show resolved Hide resolved
src/panels/MainPanel.tsx Outdated Show resolved Hide resolved
src/types/FileContent.ts Outdated Show resolved Hide resolved
src/views/PdfViewer.tsx Show resolved Hide resolved
src/atoms/core/paneGroupAtom.ts Outdated Show resolved Hide resolved
This was referenced Jun 9, 2023
@sehyod
Copy link
Collaborator Author

sehyod commented Jun 9, 2023

Thank you for these comments. I have addressed them
About the atomFamily, I decided not to use them because of this comment: pmndrs/jotai#1912 (comment), I thought it would be great to be able to iterate through open files. However, now the code is written, I realise we probably won't need it, as panes contain the list of open files, so we can iterate through these lists directly.
As you suggested, I think it would be better to refactor to use atomFamilies once we have tests. And I think this PR is already quite large so we should add tests in another PR. I have created issues for that: #128 and #129

cguedes
cguedes previously approved these changes Jun 9, 2023
@cguedes cguedes self-requested a review June 9, 2023 11:02
@sergioramos sergioramos merged commit aee0c0d into main Jun 9, 2023
8 checks passed
@sergioramos sergioramos deleted the store-file-content-in-atom branch June 9, 2023 11:08
sergioramos pushed a commit that referenced this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve TipTap editor content replacement
4 participants