refactor: refactor IDL.js into TypeScript by hansl · Pull Request #209 · dfinity/sdk

hansl · 2019-11-26T04:55:52Z

Including (but not limited to): adding types everywhere, adding missing
types for some dependencies, add prettier and changing some lint rules,
fixing the tests too, remove code that is unused, removing babel as we
dont need it, and some other changes that are too much to list.

The tests all pass.

Including (but not limited to): adding types everywhere, adding missing types for some dependencies, add prettier and changing some lint rules, fixing the tests too, remove code that is unused, removing babel as we dont need it, and some other changes that are too much to list. The tests all pass.

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-26T05:54:59Z

js-user-library/src/IDL.ts

+    throw new Error("You have the implement the method validate.");
+  }
+
+  public encode(x: any): Buffer {


Is the idea that, for each class, we should always call encode and not encodeGo? If so, we should make encodeGo private.

Why do we only validate encode type, not decode type?

Renamed validate() to covariant(), and we might add contravariant() at some point for decode.

Thanks for clarifying "Go". I'll update the code to make it private.

Actually, can't. Updated the comment.

chenyan-dfinity · 2019-11-26T06:00:40Z

js-user-library/src/IDL.ts

+}
+
+/**
+ * Represents an IDL Tuple


We don't really have IDL Tuple, I think all the comments should be "Represents JS Tuple in IDL"

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-26T16:21:19Z

js-user-library/src/IDL.test.ts

+  test_(Result, { err: 'uhoh' }, '4449444c016b029cc20171e58eb402710100010475686f68', 'Result err');
+  expect(() => IDL.encode([Result], [{}])).toThrow(/Variant has no data/);
+  expect(() => IDL.encode([Result], [{ ok: 'ok', err: 'err' }])).toThrow(/Variant has extra key/);
+  // expect(() => IDL.decode([Result], Error('Call retailerQueryAll exception: Uncaught RuntimeError: memory access out of bounds')), 'Decode error').toThrow(/Uncaught RuntimeError/)


Why do we disable this? If the bytes we are getting is an Error type, we will get a type error in the runtime?

It's a weird test... I updated it.

yep, but it's a real message we get from client while working on produceExchange...

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-26T16:44:31Z

High-level question: Is the code for IDL.js visible to end users? If so, we should make the IDL spec public along with the release. One thing I want to prevent is that people starting to treat the JS implementation as the spec and port this particular library to other languages...

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-26T23:21:45Z

js-user-library/src/IDL.ts

-  public encode(x: any): Buffer {
-    if (this.validate(x)) {
+  /** @internal */
+  public encode(x: T): Buffer {


I would suggest move encode to the public encode function, and do the covariant check there.

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-26T23:50:15Z

js-user-library/src/IDL.test.ts

+  test_(Result, { err: 'uhoh' }, '4449444c016b029cc20171e58eb402710100010475686f68', 'Result err');
+  expect(() => IDL.encode([Result], [{}])).toThrow(/Variant has no data/);
+  expect(() => IDL.encode([Result], [{ ok: 'ok', err: 'err' }])).toThrow(/Variant has extra key/);
+  // expect(() => IDL.decode([Result], Error('Call retailerQueryAll exception: Uncaught RuntimeError: memory access out of bounds')), 'Decode error').toThrow(/Uncaught RuntimeError/)


yep, but it's a real message we get from client while working on produceExchange...

chenyan-dfinity

mostly LG! just some nit.

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-27T21:19:05Z

js-user-library/src/IDL.ts

-        Buffer.concat([leb.encode(i), x.encodeType(typeTable)]),
-      ),
+      components.map((x, i) => {
+        return Buffer.concat([leb.encode(i), x.encodeType(typeTable)]);


a bit weird to have return here.

It's a function, so it's necessary. When I move the line length to 100 it will be refactored above to a single line and will be easier to read.

it doesn't have to be a function.

chenyan-dfinity · 2019-11-27T21:21:33Z

js-user-library/src/IDL.ts

-  public buildTypeTable(typeTable: TypeTable) {
-    super.buildTypeTable(typeTable);
+  public _buildTypeTableImpl(typeTable: TypeTable) {
+    super._buildTypeTableImpl(typeTable);


this._type.buildTypeTable(typeTable);

Removed the ContainerClass. It was meh.

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-27T21:26:40Z

js-user-library/src/IDL.d.ts

@@ -1,26 +0,0 @@
-import { Buffer } from "buffer/";


may want to resurrect this file.

It will be generated from the .ts file. No need for type definitions if you have a .ts file.

chenyan-dfinity · 2019-11-27T21:42:07Z

js-user-library/src/IDL.ts

+ */
+export class ArrClass<T> extends ContainerClass<T, T[]> {
+  public covariant(x: any): x is T[] {
+    return Array.isArray(x) && x.every((v) => super.covariant(v));


It would be nice to check that all values have the same type.

Do they have to, though? As far as I know they just need to be covariant (e.g. an array of Int can have Nats in it).

Most covariant arrays allow covariant item type, even if they differ.

js-user-library/src/IDL.ts

chenyan-dfinity · 2019-11-27T21:53:38Z

js-user-library/src/IDL.ts

+ * @param {Array<Type>} [argTypes] - argument types
+ * @param {Array<Type>} [retTypes] - return types
+ */
+export class FuncClass extends Type<any> {


FuncClass should not extends Type. It's not really encode/decode-able. We also get rid of the functions for covariant, encodeValue, etc.

Done. I think I needed it at some point to satisfies ActorInterface.

chenyan-dfinity · 2019-11-27T21:54:39Z

js-user-library/src/IDL.ts

+ * @returns {Buffer} serialised value
+ */
+export function encode(argTypes: Array<Type<any>>, args: any[]) {
+  if (args.length !== argTypes.length) {


args.length >= argTypes.length

Do you mean <? This is covariant.

js-user-library/src/IDL.ts

hansl requested a review from a team as a code owner November 26, 2019 04:55

chenyan-dfinity reviewed Nov 26, 2019

View reviewed changes

Hans Larsen added 3 commits November 26, 2019 13:12

fixup

f292d15

fixup

f845334

Merge branch 'master' into hansl/userlib-refactor

432fb1c

chenyan-dfinity reviewed Nov 26, 2019

View reviewed changes

Hans Larsen and others added 3 commits November 26, 2019 16:12

fixup

9cd3eeb

fixup

5c9c5da

fixup

acf27de

chenyan-dfinity reviewed Nov 27, 2019

View reviewed changes

fixup

60713d8

chenyan-dfinity approved these changes Nov 28, 2019

View reviewed changes

hansl merged commit 18e7ab0 into master Nov 28, 2019

hansl deleted the hansl/userlib-refactor branch November 28, 2019 03:19

Comments

Conversation

hansl commented Nov 26, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chenyan-dfinity commented Nov 26, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenyan-dfinity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants