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

SDK-524 Add example for JS user library #153

Merged
merged 11 commits into from
Nov 20, 2019

Conversation

paulyoung
Copy link
Contributor

This is based on #142 and paulyoung/js-user-lib-bundle so I've set the base branch to that for now.

Some things had changed since I first did this so the additional commits address those.

Hopefully the instructions in the README are all that's needed to run this.

@paulyoung paulyoung requested a review from a team as a code owner November 8, 2019 22:44
Comment on lines +1 to +24
import config from "../../dfx.json";
import actorInterface from "../../canisters/hello/main.js";

import {
generateKeyPair,
makeActor,
makeHttpAgent,
} from "@internet-computer/js-user-library";

(async () => {
const { publicKey, secretKey } = generateKeyPair();
const httpAgent = makeHttpAgent({
canisterId: config.canisters.hello.deployment_id,
senderSecretKey: secretKey,
senderPubKey: publicKey,
});
const actor = makeActor(actorInterface)(httpAgent);
try {
const reply = await actor.greet();
console.log(reply);
} catch (error) {
console.error(error);
}
})();
Copy link

Choose a reason for hiding this comment

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

I expected something more like this:

import hello from "../../canisters/hello/main.js";

(async () => {
  try {
    const reply = await hello.greet();
    console.log(reply);
  } catch (error) {
    console.error(error);
  }
})();

Obviously, this ignores the signing part but I think we want to hide things like actors and IDs from the application code... that should already be bundled into "main.js", right?

Copy link
Contributor Author

@paulyoung paulyoung Nov 11, 2019

Choose a reason for hiding this comment

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

@stanleygjones I tried to draw attention to this here: #79 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To expand on this a little bit, I don't think it should be the responsibility of the IDL compiler to produce something that can be imported directly. Doing so would mix concerns and create dependencies on the various user libraries.

I think we could potentially codegen the proceeding lines that set up the HTTP agent and actor (probably by reading some configuration for IDs, keys, etc) and expose that as something that can be imported, but only for convenience.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not following your comment here. The IDL compiler generates a form of actor signature, either in a textual form as .did file, or as a JS binding. In either case, the UserLib needs to import them. Otherwise, you have to manually write IDL types in JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm saying that I prefer the JS produced by the IDL compiler right now since it's flexible and has no dependencies.

I don't think the IDL compiler should produce code that imports the user library, or try to set up the HTTP agent and make an actor from the generated interface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Now there is a third option that JS can directly parse .did and IDL values via wasm-bindgen without the need of a JS binding (see #138). We can probably port more code from CLI this way including the HTTP agent. Not sure how far we want to go on this route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I think we should explore other options than writing IDL parsers in each language, but in this case I think it's worth trying to leverage wasm-bindgen. I'd prefer to do that an a future iteration after we get IDL.js working, since it's so close.

I'm not sure how network requests will be handled in wasm-bindgen but think that's also worth exploring in the near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stanleygjones I'm going to work on something today and propose something to address all of this

@chenyan-dfinity
Copy link
Collaborator

Related note, dfinity/motoko#881 changes the JS binding for function arguments from IDL.Obj to simply an array, which is consistent with the current design.

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

Fast tracking this.

@paulyoung paulyoung changed the base branch from paulyoung/js-user-lib-bundle to master November 20, 2019 19:14
@paulyoung paulyoung merged commit 8ced5f0 into master Nov 20, 2019
@paulyoung paulyoung deleted the paulyoung/js-user-lib-example branch November 20, 2019 19:21
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