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

Add PCDs in a folder in a single step #1364

Open
artwyman opened this issue Dec 15, 2023 · 3 comments
Open

Add PCDs in a folder in a single step #1364

artwyman opened this issue Dec 15, 2023 · 3 comments

Comments

@artwyman
Copy link
Collaborator

PCDCollection requires separate operations to add PCDs, and then set their folder. This is not only awkward to code, but it means there are intermediate states which shouldn't ever exist (where the new PCDs are in the root folder). The use of the emitter to notify listeners means that external code has a good chance of finding out about those intermediate states, including inside of internal code in PCDCollection such as applying feed actions, or merging.

I was considering some refactoring to avoid the extra emitter calls, but I think it would be cleaner to make folder an argument to addPCD() instead.

@robknight
Copy link
Member

Alternatively: restrict all changes to the PCD collection to action handlers, where no changes are committed until the action handler calls update. In the long run it's going to be a lot easier to make our action handlers atomic than to make every method on PCDCollection atomic.

The case for not caring either way would be that React only cares about updates at the next animation frame - that is, when the current event loop has finished - so the emitter firing mulitple times in the current event loop is fine in practice.

@artwyman
Copy link
Collaborator Author

I'm fascinated that we have opposite assumptions about where in the stack it's easiest to do deal with atomicity. I suspect that's driven by the different ways concurrency works in JavaScript from what I'm used to in multithreaded languages. Controlling concurrency at the app level so you don't have to at other levels is appealing, though we should talk more about how to deal with (or eliminate) state modifications which aren't inside of a dispatch action. At the app level that sometimes happens when the app needs to wait for the result of an operation. I'm also not sure if background sync wants to keep being tied to the app's action dispatch, though that's a larger design topic for later.

@ichub
Copy link
Contributor

ichub commented Jan 9, 2024

My 2c is atomicity in PCDCollection makes more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants