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 code base and build pipeline #12

Merged
merged 27 commits into from
Feb 27, 2023
Merged

Conversation

adzialocha
Copy link
Member

@adzialocha adzialocha commented Feb 14, 2023

Many things have changed since shirokuma got touched last: The p2panda-js API matured and our ideas around p2panda as well. Before we release the first version of shirokuma we want to bring it up to date with everything. This mostly concerns the "inners" of shirokuma, the actual, public API didn't change.

This PR covers the following refactors:

  • Make use of native private class fields fields (!) in JavaScript ala this.#keyPair 🤯
  • Remove (semi-automated) test fixtures and replace with hard-coded values. Maintaining this was a headache and writing simple test for shirokuma became unnecessarily complex
  • Decouple concerns: Session API, GraphQL requests, entry signing etc. are all separated now
  • Remove caching layer: Since we introduced querying the next entry arguments based on document view ids the current caching logic doesn't work anymore. Caching keys are currently organized as ${publicKey}/${viewId}, which means that it will just make an entry in the cache after every next entry request but never hit afterwards as the view id changes constantly. To bring back caching we need to extend the API with something which allows us to handle document instances, this can be handled in future PRs: Cache next entry arguments #13
  • Return "local document view id" instead of Session instance when creating, updating, deleting documents: Return hash of published entry from create(), update() and delete() methods on session #5
  • Removing "marshalling" logic, all of this is now handled by p2panda-js
  • p2panda-js offers a "slim" and "inlined" version, where the latter has the WebAssembly code inlined. We want to also make use of these options in shirokuma. To enable this the rollup build pipeline has been improved
  • Removed logging (for now): With the codebase becoming simpler now I couldn't find a reason to log every request and potential error, both of them you can see in your regular inspector tools

Thoughts on the build pipeline:

Currently we do not bundle external dependencies in our CommonJS, ESM and NodeJS builds. Only the UMD builds contain the whole thing. I couldn't find any "best practices" yet on how to do this but hopefully with a modern JS environment these decisions make sense: Webpack, Rollup etc. handles this automatically for us and if we want to go fancy ESM in the browser via <script type="module"> we load the external dependencies manually before (will write something about it in the README.md). The UMD build should cover the needs of developers who "don't care" and simply just pull something in to play with.

Todo

  • Fix JSON-import assertion error of tsc
  • Find out why entries are differently encoded in js world .. (in the fixtures the log id was still starting at 1!)
  • Remove caching layer for now
  • Add back all the tests (w. refactored fixtures)
  • Test everything w. latest (unreleased) p2panda-js version
  • Add back all the doc strings

Follow ups

Closes #8, #9, #5

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Base: 96.63% // Head: 92.10% // Decreases project coverage by -4.54% ⚠️

Coverage data is based on head (da4ef85) compared to base (0fd59b0).
Patch coverage: 92.03% of modified lines in pull request are covered.

❗ Current head da4ef85 differs from pull request most recent head 7accede. Consider uploading reports for the commit 7accede to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
- Coverage   96.63%   92.10%   -4.54%     
==========================================
  Files          11        5       -6     
  Lines         238      114     -124     
  Branches       40       19      -21     
==========================================
- Hits          230      105     -125     
- Misses          8        9       +1     
Impacted Files Coverage Δ
src/graphql.ts 73.91% <73.91%> (ø)
src/session.ts 95.65% <95.65%> (ø)
src/entry.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/operation.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adzialocha adzialocha marked this pull request as ready for review February 21, 2023 11:18
@adzialocha
Copy link
Member Author

adzialocha commented Feb 21, 2023

Coverage report is sad, but I'm not sure if winning the coverage award helps here .. all the other methods are barely wrappers around p2panda-js (which is tested). I guess adding some simple unit tests checking against the missing arguments will be enough. What do you think?

@sandreae
Copy link
Member

Coverage report is sad, but I'm not sure if winning the coverage award helps here .. all the other methods are barely wrappers around p2panda-js (which is tested). I guess adding some simple unit tests checking against the missing aruments will be enough. What do you think?

Yeh, I don't think this is necessarily the place to fight the coverage wars 😉

I can do a review of this PR tomorrow 👍 I'm looking forward to it!

Copy link
Member

@sandreae sandreae left a comment

Choose a reason for hiding this comment

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

This all looks really great, I like how the api is now exposing some of the lower-level functionality while still keeping the high level Session as the main interface.

The build pipeline is wild, fantastic to support so many targets and have a well considered flow for that.

My only comments were about clarification in doc strings I think.

README.md Show resolved Hide resolved
src/operation.ts Outdated Show resolved Hide resolved
src/session.ts Outdated Show resolved Hide resolved
src/session.ts Outdated Show resolved Hide resolved
src/session.ts Outdated Show resolved Hide resolved
@adzialocha adzialocha merged commit 129f850 into main Feb 27, 2023
@adzialocha adzialocha deleted the clean-and-improve-api branch February 27, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants