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

Js client first implementation #192

Merged
merged 9 commits into from
Sep 25, 2018
Merged

Js client first implementation #192

merged 9 commits into from
Sep 25, 2018

Conversation

DieMyst
Copy link
Member

@DieMyst DieMyst commented Sep 24, 2018

No description provided.

Copy link
Member

@alari alari left a comment

Choose a reason for hiding this comment

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

  • There's a list of minor issues
  • Probably you should migrate to Webpack
  • Code mostly looks like work-in-progress
  • I think we should merge it after the tiny fixes, as it will evolve along with the ongoing VM and Consensus updates

@@ -0,0 +1,120 @@
## IMPORTANT: Client and README are under heavy development and can be outdated. Ask questions in gitter or in issues.
Copy link
Member

Choose a reason for hiding this comment

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

Ask a question in gitter or file an issue

* limitations under the License.
*/

var gulp = require("gulp");
Copy link
Member

Choose a reason for hiding this comment

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

You should use webpack.

js-client/src/Result.ts Outdated Show resolved Hide resolved
* @param responseTimeoutSec what time to check
*/
async result(requestsPerSec: number = 4, responseTimeoutSec = 5): Promise<Result> {
const path = wrapInQuotes(this.target_key + "/result");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's safer to use JSON.stringify instead? So that quotes inside the string are escaped?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right!

let engine = new Engine(tm);

// default signing key for now
let signingKey = "TVAD4tNeMH2yJfkDZBSjrMJRbavmdc3/fGU2N2VAnxT3hAtSkX+Lrl4lN5OEsXjD7GGG7iEewSod472HudrkrA==";
Copy link
Member

Choose a reason for hiding this comment

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

Probably some TODO with explanation should be there?

}
}

const _global = (window /* browser */ || global /* node */) as any;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you do it? Why not to simply export the class, and then import it where it's needed?

* @param host
* @param port
*/
export async function defaultTest(host: string, port: number) {
Copy link
Member

Choose a reason for hiding this comment

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

This test could never fail? Looks like the naming is wrong.

return wrapInQuotes(utils.toHex(Buffer.from(JSON.stringify(json))).toUpperCase())
}

class TxCl {
Copy link
Member

Choose a reason for hiding this comment

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

Some docs expected.

*/

export function wrapInQuotes(str: string): string {
return "\"" + str + "\""
Copy link
Member

Choose a reason for hiding this comment

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

str could contain quotes inside. Will it be safe in this case?

@DieMyst DieMyst merged commit 13711d7 into master Sep 25, 2018
@DieMyst DieMyst deleted the js-client-impl branch September 25, 2018 12:52
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.

2 participants