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

Feat/clarinet sdk browser #1448

Merged
merged 6 commits into from
Jul 12, 2024
Merged

Feat/clarinet sdk browser #1448

merged 6 commits into from
Jul 12, 2024

Conversation

hugocaillard
Copy link
Collaborator

@hugocaillard hugocaillard commented May 3, 2024

Description

Closes: #1432

This is a pretty important PR that brings the Clarinet SDK for the browser.

Two new packages

  • @hirosystems/clarinet-sdk-wasm-browser
  • @hirosystems/clarinet-sdk-browser

They are the equivalent of these existing packages (but for the browser)

  • @hirosystems/clarinet-sdk-wasm
  • @hirosystems/clarinet-sdk

Enhancement

This PR also brings some enhancement to the SDK and some new methods useful for the browser use case (such as initEmptySession() and clearCache()).

Improved typing

Some Rust structs are encoded in JS objects, and the TS types for those were in clarinet-sdk.
They have been moved to components/clarinet-sdk-wasm/src/ts_types.rs

Review

A good first step to review this PR is to look at these 4 READMEs:

Some docs should be moved to the root Readme (how to build the sdk), but this readme require a lot of work, let's do this in a future PR

Caveats

It brings complexity to an already complicated architecture.

The clarinet-sdk-wasm component remain almost the same, except that it now requires a build script that runs wasm-pack twice targeting nodejs and web. It will also rename the web package to @hirosystems/clarinet-sdk-wasm-browser.

npm run build:wasm

The src directory of the clarinet-sdk component has been split into 3 directories common/src, node/src, browser/src. With a bit of code duplication between node/src/sdkProxy.ts and browser/src/sdkProxy.ts.
But thanks to NPM workspace, it still easy to build and publish the two components.

Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 37.68116% with 172 lines in your changes missing coverage. Please review.

Files Patch % Lines
components/clarity-repl/src/repl/session.rs 37.59% 161 Missing ⚠️
components/clarity-repl/src/frontend/terminal.rs 0.00% 10 Missing ⚠️
components/clarity-repl/src/bin.rs 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@hugocaillard hugocaillard self-assigned this May 6, 2024
@hugocaillard hugocaillard marked this pull request as ready for review May 6, 2024 15:44
components/clarinet-sdk-wasm/README.md Outdated Show resolved Hide resolved
components/clarinet-sdk-wasm/README.md Show resolved Hide resolved
components/clarinet-sdk/README.md Outdated Show resolved Hide resolved
components/clarinet-sdk-wasm/src/core.rs Outdated Show resolved Hide resolved
components/clarinet-sdk/README.md Show resolved Hide resolved
@hugocaillard hugocaillard marked this pull request as draft June 7, 2024 14:37
@hugocaillard hugocaillard force-pushed the feat/clarinet-sdk-browser branch 2 times, most recently from 8ed2484 to 859957a Compare June 12, 2024 14:02
@hugocaillard hugocaillard force-pushed the feat/clarinet-sdk-browser branch 3 times, most recently from dbe51ff to d6e3272 Compare June 25, 2024 14:23
@hugocaillard hugocaillard force-pushed the feat/clarinet-sdk-browser branch 3 times, most recently from 10fd20c to 46105b9 Compare July 3, 2024 08:13
@hugocaillard hugocaillard removed the request for review from MicaiahReid July 8, 2024 16:59
@hugocaillard hugocaillard marked this pull request as ready for review July 8, 2024 16:59
janniks
janniks previously approved these changes Jul 9, 2024
Copy link
Contributor

@janniks janniks left a comment

Choose a reason for hiding this comment

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

Looking good 👏

components/clarinet-sdk-wasm/README.md Show resolved Hide resolved
components/clarinet-sdk/browser/README.md Outdated Show resolved Hide resolved
components/clarinet-sdk/browser/package.json Show resolved Hide resolved
components/clarinet-sdk/node/src/sdkProxy.ts Outdated Show resolved Hide resolved
components/clarity-repl/src/repl/session.rs Show resolved Hide resolved
components/clarity-repl/src/repl/session.rs Show resolved Hide resolved
CharlieC3
CharlieC3 previously approved these changes Jul 9, 2024
Copy link
Member

@CharlieC3 CharlieC3 left a comment

Choose a reason for hiding this comment

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

One minor suggestion for the sdk workflow. Otherwise looks good!

.github/workflows/ci-sdk.yaml Outdated Show resolved Hide resolved
@hugocaillard hugocaillard dismissed stale reviews from CharlieC3 and janniks via 3fea703 July 9, 2024 14:02
CharlieC3
CharlieC3 previously approved these changes Jul 9, 2024
Copy link
Member

@CharlieC3 CharlieC3 left a comment

Choose a reason for hiding this comment

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

Approving GHA workflow changes/additions

@hugocaillard
Copy link
Collaborator Author

Thanks @CharlieC3

@hugocaillard hugocaillard requested a review from janniks July 9, 2024 14:37
janniks
janniks previously approved these changes Jul 9, 2024
tippenein
tippenein previously approved these changes Jul 11, 2024
@hugocaillard hugocaillard dismissed stale reviews from tippenein and janniks via ee904cd July 11, 2024 08:25
@hugocaillard hugocaillard enabled auto-merge (squash) July 11, 2024 08:32
@hugocaillard hugocaillard merged commit 1982441 into main Jul 12, 2024
19 checks passed
@hugocaillard hugocaillard deleted the feat/clarinet-sdk-browser branch July 12, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Publish a clarinet-sdk package for the browser
5 participants