[WIP] Binding gen #1287
[WIP] Binding gen #1287
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Shaptic
left a comment
Shaptic
left a comment
There was a problem hiding this comment.
Looks awesome so far! Mostly minor comments below, gonna try it out right now too.
scripts/download-sac-spec.sh
Outdated
| @@ -0,0 +1,62 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
since this is user-facing we probably want it written in pure JS so our 3 windows users aren't in shambles
There was a problem hiding this comment.
Good call. Just want to call out that in most ecosystems and in many regions windows users are more common than other operating systems, so I think there are many more than 3.
There was a problem hiding this comment.
I know :) it was tongue-in-cheek, we should def do it
| * Generate .gitignore | ||
| */ | ||
| private generateGitignore(): string { | ||
| const gitignore = [ |
There was a problem hiding this comment.
Should we just fetch and write the official one, instead?
src/bindings/client.ts
Outdated
| const optionType = this.generateTypeFromTypeDef( | ||
| typeDef.option().valueType(), | ||
| ); | ||
| return `${optionType} | undefined`; |
There was a problem hiding this comment.
perhaps?
| return `${optionType} | undefined`; | |
| return `${optionType} | undefined | null`; |
| @@ -0,0 +1,4 @@ | |||
| // Auto-generated by scripts/download-sac-spec.sh | |||
| // Do not edit manually - run the script to update | |||
| export const SAC_SPEC = | |||
There was a problem hiding this comment.
Should we download & generate this as part of the generator if the command detects that it's a SAC rather than bundling it in?
There was a problem hiding this comment.
I would favor making this deterministic over live fetching. In protocol 23 muxed addresses were added. Live fetching would have broken the cli if it had fetched a sac contract spec that contained a type it didn't know about.
| } | ||
| if (!args.networkPassphrase) { | ||
| throw new WasmFetchError( | ||
| "--network-passphrase is required when fetching from network", |
There was a problem hiding this comment.
Can/should we have sensible defaults for the URL given the passphrase, or prefer a simpler --network testnet|pubnet|futurenet option that has these prefilled?
Shaptic
left a comment
Shaptic
left a comment
There was a problem hiding this comment.
Couple more thoughts after running it on a side-project of mine
src/bindings/client.ts
Outdated
| * Generate interface method signature | ||
| */ | ||
| private generateInterfaceMethod(func: any): string { | ||
| const name = func.name().toString(); |
There was a problem hiding this comment.
We could be careful here to ensure these don't conflict with JS keywords, thoughts? A lot of these will already be covered by Rust when they write the contract, granted, but worth a thought.
src/bindings/client.ts
Outdated
|
|
||
| const params = this.formatMethodParameters(inputs); | ||
|
|
||
| return ` ${name}(${params}): Promise<AssembledTransaction<${outputType}>>;`; |
There was a problem hiding this comment.
Should we also inject the docs pulled from the spec for each method?
| const types = typeGenerator.generate(); | ||
| const client = clientGenerator.generate(); | ||
|
|
||
| const index = `export { Client } from "./client.js";`; |
There was a problem hiding this comment.
Should we export it with the name prefixed? They may have multiple clients so this would provide clarity and avoid conflicts.
There was a problem hiding this comment.
We need to balance this with it being drop in replacement for the current binding generation.
There was a problem hiding this comment.
Good call, we can export twice then and break later
|
Size Change: +2.25 MB (+5.29%) 🔍 Total Size: 44.7 MB
|
Move typescript binding generation from the stellar-cli to the js-stellar-sdk #1154