-
Notifications
You must be signed in to change notification settings - Fork 352
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: upgrade to substrate-connect v0.8.4 #5776
feat: upgrade to substrate-connect v0.8.4 #5776
Conversation
const addChain = this.#sharedSandbox | ||
? (async (...args) => (await this.#sharedSandbox!.#chain)!.addChain(...args)) as ScType.AddChain | ||
: this.#wellKnownChains.has(this.#spec as ScType.WellKnownChain) | ||
? client.addWellKnownChain | ||
: client.addChain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The this.#sharedSandbox!.#chain
is not transpiled properly see
I think this is related to this polkadot-js/dev
script
https://github.com/polkadot-js/dev/blob/1d7db82109f666dd013d5db04ef2bd6a45f4d74a/packages/dev/scripts/polkadot-dev-build-ts.mjs#L74-L84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an alternative that is coupled to a polkadot-dev-build-ts.mjs
implementation detail, this can be re-written to
const addChain = this.#sharedSandbox
? (async (...args) => {
const source = this.#sharedSandbox!;
return (await source.#chain)!.addChain(...args);
}) as ScType.AddChain
: this.#wellKnownChains.has(this.#spec as ScType.WellKnownChain)
? client.addWellKnownChain
: client.addChain;
and it will transpile to
const addChain = this.__internal__sharedSandbox
? (async (...args) => {
const source = this.__internal__sharedSandbox;
return (await source.__internal__chain).addChain(...args);
})
: this.__internal__wellKnownChains.has(this.__internal__spec)
? client.addWellKnownChain
: client.addChain;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the dev script implementation can actually be dropped (after we dropped Node 16 last year). The issue is that TSC compiled everything #<id>
to WeakMaps for Node 16 compat and it had severe implications for performance. So at best it is/was a horrible workaround.
So the adjustment will be dropped sometime, but no idea when that sometime will be - needs some checks all over when done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for the info @jacogr
I'll re-write it as suggested previously so it's more friendly with this script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-written in 7df6a77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small CI linting issues - should not be a massive issue to resolve.
const addChain = this.#sharedSandbox | ||
? (async (...args) => { | ||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
const source = this.#sharedSandbox!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint-disable
because it's checked on line 157
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems spot-on. Please just merge master to resolve the yarn.lock
conflicts.
The conflicts with the yarn.lock
file have been addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kratico ! I will try to find out how to create a release.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Upgrade
@substrate/connect
tov0.8.4
and use the newaddChain
API.The
addChain
API uses the relay chain to instantiate new parachains, for exampleGiven that the
ScProvider
is injected a relay chain provider, no changes are needed in theScProvider
public API and it works as before, for exampleNote: given that
ScProvider
receivescreateScClient
from@substrate/connect
as input to the constructor and that the@substrate/connect
API has a breaking change, this is also a breaking change for@polkadot/rpc-provider
.