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

chore: update substrate connect readme #1715

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

kratico
Copy link
Contributor

@kratico kratico commented Jan 3, 2024

No description provided.

@kratico kratico force-pushed the chore/update-substrate-connect-readme branch from c4bf4de to c499c18 Compare January 3, 2024 13:27
@kratico kratico requested a review from josepot January 3, 2024 13:27
@kratico kratico changed the title Chore/update substrate connect readme chore: update substrate connect readme Jan 3, 2024
Copy link
Contributor

@wirednkod wirednkod left a comment

Choose a reason for hiding this comment

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

How does, removing all previous documentation without adding new one, helps explaining how to use Substrate connect with either PAPI or polkadotJS?

@kratico kratico force-pushed the chore/update-substrate-connect-readme branch from b26fae2 to 1831e1c Compare January 3, 2024 15:13
Copy link
Contributor

@josepot josepot left a comment

Choose a reason for hiding this comment

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

Thanks!

@kratico
Copy link
Contributor Author

kratico commented Jan 3, 2024

How does, removing all previous documentation without adding new one, helps explaining how to use Substrate connect with either PAPI or polkadotJS?

Thanks for your concern @wirednkod
As discussed offline, it might not scale well to add documentation for each library that might use @substrate/connect.
Instead, we could add examples in the projects/ folder or open PRs in https://github.com/paritytech/polkadot-api or https://github.com/polkadot-js/api or other libraries with @substrate/connect examples,

@kratico kratico enabled auto-merge (squash) January 3, 2024 17:13
@kratico kratico force-pushed the chore/update-substrate-connect-readme branch from 1831e1c to fe66c67 Compare January 3, 2024 17:13
@kratico kratico requested a review from wirednkod January 3, 2024 17:14
@wirednkod wirednkod removed their request for review January 3, 2024 17:14
@josepot
Copy link
Contributor

josepot commented Jan 3, 2024

How does, removing all previous documentation without adding new one, helps explaining how to use Substrate connect with either PAPI or polkadotJS?

It's the responsibility of those libraries to explain to their users how to build light-client dApps. In the case of PJS, it probably would make sense to improve this section of their docs.

@substrate/connect is a fairly low-level library that provides a basic interface to interact with smoldot. So, its docs should document that interface. The libraries/frameworks building on top of it are responsible for educating its consumers on how to build light-client dApps.

@wirednkod
Copy link
Contributor

we could add examples in the projects/ folder or open PRs in paritytech/polkadot-api or polkadot-js/api or other libraries with @substrate/connect examples,

This is what I try to explain offline as well for long. No need to remove docs and alter demos.
Just add new docs for the PAPI versions, and create new demos, clarifying the support for these 2 APIs. IMO the repo at this point is super confusing.

@kratico
Copy link
Contributor Author

kratico commented Jan 3, 2024

@wirednkod @substrate/connect docs should not be coupled to any dependent library.

PolkadotAPI or PolkadotJS or any other, may chose to support the latest (or an specific) version for @substrate/connect like any other library that depends on a package.

Also, once polkadot-js/api#5776 is merged, a new PolkadotJS example with parachains could be added, for example

import { ApiPromise } from "@polkadot/api"
import { ScProvider } from "@polkadot/rpc-provider"
import * as Sc from "@substrate/connect"
import jsonParachainSpec from "./src/assets/asset-hub-westend.json" assert { type: "json" }

const parachainSpec = JSON.stringify(jsonParachainSpec)

const relayProvider = new ScProvider(Sc, Sc.WellKnownChain.westend2)
const provider = new ScProvider(Sc, parachainSpec, relayProvider)

await provider.connect()

const polkadotApi = await ApiPromise.create({ provider })
await polkadotApi.rpc.chain.subscribeNewHeads((lastHeader) => {
  console.log(lastHeader.number.toString())
})

@josepot
Copy link
Contributor

josepot commented Jan 3, 2024

we could add examples in the projects/ folder or open PRs in paritytech/polkadot-api or polkadot-js/api or other libraries with @substrate/connect examples,

This is what I try to explain offline as well for long. No need to remove docs and alter demos. Just add new docs for the PAPI versions, and create new demos, clarifying the support for these 2 APIs. IMO the repo at this point is super confusing.

We are of the opinion that it's not a good idea to document in the README of @substrate/connect how it can be used with PJS and/or PAPI. The reason why we think that it's not a good idea is double-fold:

  • On one end because the APIs of those libraries could change, and if they did then this README would be providing outdated information. This is something that has already happened in the past.
  • Also, consider the current situation: we just made a breaking change and we just don't know (as of right now), how these 2 different libraries will be leveraging this breaking change. It's very possible that this could result in a different API on PJS, actually, but we don't know it yet, so we just can't document it. Also, with regards of PAPI: currently all of those APIs are highly experimental, they will get stabilized during Q1. However, again, what it makes sense is for each library to document their own API.

Also, it's worth pointing out that in an ideal world the average dApp developer wouldn't know about the existence of this library, since this library should just be an internal implementation detail of a more developer-friend API that abstracts all this complexity away. So, I'm afraid that we see things quite differently.

All that being said, we will try our best to explain and document all these things as soon as we have the bandwidth to do so 🙂.

@josepot josepot requested review from wirednkod and removed request for wirednkod January 3, 2024 17:41
@wirednkod wirednkod removed their request for review January 3, 2024 17:41
Copy link
Contributor

@wirednkod wirednkod left a comment

Choose a reason for hiding this comment

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

Feel free to proceed as you wish. This is not my project nor have any saying on the roadmap.

@kratico kratico merged commit 2a62669 into main Jan 3, 2024
9 checks passed
@kratico kratico deleted the chore/update-substrate-connect-readme branch January 3, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants