Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

Cells #221

Merged
merged 68 commits into from
Jun 16, 2022
Merged

Cells #221

merged 68 commits into from
Jun 16, 2022

Conversation

voltrevo
Copy link
Collaborator

@voltrevo voltrevo commented Jun 7, 2022

Video walkthrough

Video walkthrough

What is this PR doing?

Adds a new internal library called cells. See README.md for more information.

How can these changes be manually tested?

Visit /quillPage.html#/cells-demo and try it out.

Does this PR resolve or contribute to any issues?

None specifically (but it should contribute to the resolution of a high percentage of future ui-related issues).

Checklist

  • I have manually tested these changes
  • Post a link to the PR in the group chat

Guidelines

  • If your PR is not ready, mark it as a draft
  • The resolve conversation button is for reviewers, not authors
    • (But add a 'done' comment or similar)

@github-actions github-actions bot added the extension Browser extension related label Jun 7, 2022
@voltrevo voltrevo changed the base branch from main to bw-173-214-215-fix-rpc June 7, 2022 04:45
@voltrevo voltrevo changed the base branch from bw-173-214-215-fix-rpc to main June 7, 2022 04:45
@voltrevo voltrevo changed the base branch from main to bw-173-214-215-fix-rpc June 7, 2022 04:46
Base automatically changed from bw-173-214-215-fix-rpc to main June 9, 2022 14:44
Copy link
Collaborator

@jacque006 jacque006 left a comment

Choose a reason for hiding this comment

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

Asides from my in code comments, I think a core choice we need to make for the extension is what kind of data management/lifecycle (model/controller) framework we want to use. Using our current controller/object state approach was a good start but we are growing beyond that. I see two main paths.

Pros of using cells or a custom internal data framework

  • We have complete control/customization of our data modeling layer.
  • Allows some ease/utility in running more complex async calculations for derived data.
  • Already implemented in extension codebase :)

Pros of using redux or a similar framework

  • Heavily tested, wide use in production projects.
  • Ecosystem support (middleware, add-ons, exisiting examples)
  • Used by exisiting web wallets (Metamask, Tally), easier to show BLS Wallet examples to those wallet devs.
  • Easier to onboard new developers, accept external contributions with understanding of flux patterns.

Some other general thoughts

  • We should use properties rather than string keys to access cells/data so we can leverage TS to enforce type usage.
  • Using io-ts is handy when we need to accept external inputs or validate rehydrated data from storage, but seems like overkill to use everywhere internally when can use TS to control read/write types. If will also add runtime overhead to check those types over and over again.
  • I like the versioned data, and think we should stick with that pattern when loading from storage.
  • There are a ton of commits (> # of files changed) in this branch that will flood our commit history. Worth merging as a rebase or consolidating some commits.

extension/source/QuillPage/CellsDemo/BalanceWidget.tsx Outdated Show resolved Hide resolved
extension/source/QuillPage/CellsDemo/BalanceWidget.tsx Outdated Show resolved Hide resolved
extension/source/QuillPage/CellsDemo/CellsDemoPage2.tsx Outdated Show resolved Hide resolved
extension/source/QuillPage/index.tsx Outdated Show resolved Hide resolved
extension/source/QuillPage/CellsDemo/TextBox.tsx Outdated Show resolved Hide resolved
extension/source/QuillPage/QuillContext.tsx Show resolved Hide resolved
extension/source/QuillPage/QuillPage.tsx Outdated Show resolved Hide resolved
extension/source/QuillPage/TimeCell.ts Show resolved Hide resolved
extension/source/cells/CellCollection.ts Show resolved Hide resolved
extension/source/cells/CellCollection.ts Show resolved Hide resolved
@voltrevo
Copy link
Collaborator Author

voltrevo commented Jun 15, 2022

Pros of using redux or a similar framework

  • Heavily tested, wide use in production projects.
  • Ecosystem support (middleware, add-ons, exisiting examples)
  • Used by exisiting web wallets (Metamask, Tally), easier to show BLS Wallet examples to those wallet devs.
  • Easier to onboard new developers, accept external contributions with understanding of flux patterns.

While I acknowledge the above points, my dominant thought here is that I'd actually prefer to use an existing library if it allows us to more easily express the same ideas:

  • Share a reference to an identity with interior mutability (not a sequence of updates)
  • Access to the internal value is asynchronous
  • The value is set to a default if it doesn't exist, but this is not a substitute for having not yet loaded
  • Built for TypeScript, with types derived from runtime type information and types checked at runtime
  • A way to compose these identities with a function to create a read-only identity which is only executed when needed

To my knowledge, there isn't a good substitute. Indeed, this is the reason why I'm rather excited about cells - I think this is a really compelling abstraction that has potential to inspire others to build upon it and bring attention to blswallet.

Additionally, I think there is a psychological factor to consider here. I'm a human, not a robot, and I'm the most active contributor to the extension (in recent times, although it's probably true in the lifetime of the extension too). My productivity is linked to being passionate, and I'm passionate about this. It's not a very good reason, and I would never ask anyone to accept my code if I didn't also believe it was separately a good idea, but I believe this reason is worth factoring in.

  • Using io-ts is handy when we need to accept external inputs or validate rehydrated data from storage, but seems like overkill to use everywhere internally when can use TS to control read/write types. If will also add runtime overhead to check those types over and over again.

I guess I'm a bit paranoid at the moment because of the amount of inaccurate types we have flying around because of #42. I think the overhead is worth it for now, and indeed the overhead is kindof a non-issue. Happy to chat more about it though.

  • There are a ton of commits (> # of files changed) in this branch that will flood our commit history. Worth merging as a rebase or consolidating some commits.

I've never quite understood this. Having the detail is useful when things go sideways and you want to explore how the code came to be. You can still look at the pull requests or merge commits if you want a coarse/curated history instead.

I've always preferred keeping the original history. Having said that, I don't mind too much if the team prefers squashing.

Oh and another thought - I believe that build artifacts should reference the git sha, allowing you to see what they were built from. Squashing causes these to be deleted. We actually ran into this previously with bls-wallet-clients.

@voltrevo voltrevo mentioned this pull request Jun 15, 2022
1 task
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 15, 2022
@jacque006
Copy link
Collaborator

Summary from offline conversation with @voltrevo :

  • Using cells over another framework as @voltrevo is very passionate about it's use and thinks we can use it effectively without slowing down/complicating new devs onboarding or external contributions. We may eventually extract it out of this repo if it gets general enough.
  • There is ongoing followup work to optimize or fix a number of mentioned issues in follow up PRs
  • Unit tests will be added in future work.
  • We will consider in future iterations being a little less heavy on io-ts in certain areas and in parts of the runtime checks as makes sense.
  • This PR will be squashed to limit # of commits from this PR.

I will create some follow up issues for the above.

@jacque006 jacque006 merged commit e671e73 into main Jun 16, 2022
@jacque006 jacque006 deleted the cells branch June 16, 2022 00:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation extension Browser extension related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants