Skip to content

refactor: store file content in atom#105

Merged
sergioramos merged 19 commits into
mainfrom
store-file-content-in-atom
Jun 9, 2023
Merged

refactor: store file content in atom#105
sergioramos merged 19 commits into
mainfrom
store-file-content-in-atom

Conversation

@sehyod

@sehyod sehyod commented Jun 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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
Closed
4 tasks
@hammer hammer mentioned this pull request Jun 8, 2023
Closed
danvk
danvk requested changes Jun 8, 2023
View reviewed changes

@danvk danvk left a comment

Copy link
Copy Markdown
Collaborator

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 #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.

Comment thread src/atoms/core/activePaneAtom.ts Outdated
Comment thread src/atoms/core/fileContentAtom.ts Outdated
Comment thread src/atoms/core/fileContentAtom.ts Outdated
Comment thread src/atoms/core/fileContentAtom.ts Outdated
Comment thread src/atoms/core/fileContentAtom.ts Outdated
Comment thread src/panels/MainPanel.tsx Outdated
Comment thread src/panels/MainPanel.tsx Outdated
Comment thread src/types/FileContent.ts Outdated
Comment thread src/views/PdfViewer.tsx
Comment thread src/atoms/core/paneGroupAtom.ts Outdated
This was referenced Jun 9, 2023
Closed
Open
@sehyod

sehyod commented Jun 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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
View reviewed changes
@cguedes cguedes self-requested a review June 9, 2023 11:02
@sergioramos sergioramos merged commit aee0c0d into main Jun 9, 2023
@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
@danvk
chore: lint JSDoc with eslint-plugin-jsdoc (#124)
508564e 
references: #105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danvk danvk danvk requested changes
@cguedes cguedes cguedes approved these changes

Assignees

@sehyod sehyod

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve TipTap editor content replacement

4 participants

@sehyod @danvk @cguedes @sergioramos