Refactor the project and add generics and getter options #213

petarvujovic98 · 2022-09-09T14:05:30Z

I used basic eslint and prettier installs without any configs to format the files.

There were also function refactors for better maintainability and JS standards as well as variable renames.

I also added generics to collections as well as a optional getter options object which has a reconstructor and default value property.

ailisp · 2022-09-13T09:10:50Z

Thank you @petarvujovic98 !

I used basic eslint and prettier installs without any configs to format the files.
There were also function refactors for better maintainability and JS standards as well as variable renames.

These parts are great! Function refactor and variable renames also look good.

I also added generics to collections as well as a optional getter options object which has a reconstructor and default value property.

This seems already in #203 , would you mind wait that PR merge and doing a rebase? I believe except #203, there's no other PR that should merge before this one to avoid conflictions.

ailisp · 2022-09-13T09:12:40Z

Also, just a small suggestion: it'll be great to separate the refactor commit with the reformatting next time, to make code review easier

petarvujovic98 · 2022-09-13T09:12:46Z

@ailisp Of course, I'll wait for the generics merge and rebase this PR.

ailisp · 2022-09-13T09:13:48Z

@volovyks CI was not triggering for this PR, do you know why

volovyks · 2022-09-13T15:03:24Z

@ailisp that is because @petarvujovic98 is not a part of our GitHub org. I have started a discussion about it in Slack.

petarvujovic98 · 2022-09-13T19:00:30Z

@ailisp I just saw this comment now, I could separate the reformat and refactor after I rebase the code. This was just me going fast

ailisp · 2022-09-15T09:20:34Z

@petarvujovic98 Sorry looks like a lot of conflictions are in! It may be faster to have your function refactor changes first. Then we can re-apply your formatting to new develop branch in another PR.

petarvujovic98 · 2022-09-15T09:25:56Z

@ailisp Sounds good. Will do the refactor on Monday

Add ESLint and Prettier packages and configs Run ESLint and Prettier Add ESLint and Prettier scripts

Change overrides in ESLint

Add includeBytes function declaration Add helper for returning value from collection methods Add helper types Refactor code

Add defaultValue to methods that return values Add better type definitions Refactor code

Rename variables and parameters to JavaScript standards Refactor code

Improve type definitions for QJS environment Rename variables and parameters to JavaScript standards Refactor code

Improve readability of code Add comments for easier understanding Marginaly improve decorator type definitions

petarvujovic98 · 2022-09-19T20:47:53Z

@ailisp @volovyks I just did a refactor of this PR. Please review and add any comments on the changes.

ailisp · 2022-09-20T03:06:20Z

.eslintrc.cjs

+module.exports = {
+  root: true,
+  env: {
+    es2021: true,


The underlying engine quickjs only support es2020: https://bellard.org/quickjs/ so we need es2020 here

@ailisp This is just for the code we are writing, doesn't have to do anything with the target for the TypeScript compilation. So we are using newer features in code, or at least we could be, but then the TypeScript compiler will build them to whatever version we choose in the tsconfig.json file.

ailisp · 2022-09-20T03:07:09Z

README.md

-    - if arguments more than params, remaining argument are ignored
-    - if argument is different than the required type, it'll be coerced to required type
-    - if argument is different than the required type but cannot be coerced, will throw runtime type error, also with message and stacktrace
+When call host function with inappropriate type, means incorrect number of arguments or arg is not expected type: - if arguments less than params, remaining argument are set as 'undefined' - if arguments more than params, remaining argument are ignored - if argument is different than the required type, it'll be coerced to required type - if argument is different than the required type but cannot be coerced, will throw runtime type error, also with message and stacktrace


This formatting seems problematic, it was a bulleted list

@ailisp Thanks, haven't noticed it went wrong. Will fix formatting.

ailisp · 2022-09-20T03:15:13Z

src/api.ts

 import { PromiseResult } from "./types";

 const U64_MAX = 2n ** 64n - 1n;
 const EVICTED_REGISTER = U64_MAX - 1n;

 // Interface available in QuickJS
 interface Env {
-  panic_utf8: (msg: string) => never;
-  [x: string]: any;
+  panic_utf8(msg: string): never;


Nice additions to the env interface!

most of the functions here should take Bytes instead of string, please referring to the implementation function below. For example,

export function panicUtf8(msg: Bytes): never { env.panic_utf8(msg); }

so env.panic_utf8 should take a Bytes

@ailisp Will update those definitions to take Bytes instead.

ailisp · 2022-09-20T03:16:25Z

src/api.ts

 ): bigint {
  return env.promise_create(accountId, methodName, args, amount, gas);
 }

 export function promiseThen(
-  promiseIndex: number | bigint,
+  promiseIndex: PromiseIndex,


Seems you also fixed NearAmount and PromiseIndex typings. That's great!

@ailisp This does not actually fix the #118 problem. Maybe I could create another PR after this one to address this. For now this only aliases number | bigint to PromiseIndex. We should first agree on a good API for this (I'll have a better look at the Rust implementation for inspiration).

ailisp · 2022-09-20T03:17:11Z

src/api.ts

+    `Promise result ${
+      status == PromiseResult.Failed
+        ? "Failed"
+        : status == PromiseResult.NotReady


Nice refactor!

ailisp · 2022-09-20T03:19:47Z

src/build-tools/near-bindgen-exporter.js

+                    readState(classId),
+                    // Throw if initialized on any subsequent init function calls.
+                    // if (_state) { throw new Error('Contract already initialized'); }
+                    preventDoubleInit(contractMethods[methodName]),


These helper code generators are really cool! Easier to read then our previous version. cc @volovyks

ailisp · 2022-09-20T03:21:06Z

src/collections/unordered-map.ts

    // reconstruct keys Vector
-    map.keys = new Vector(data.prefix + "u");
+    map.keys = new Vector(`${data.prefix}u`);


Is this better than +?

@ailisp It is not better in any performance or conventions, I just prefer it to be template literals, we are using them elsewhere in code so thought might as well keep it the same all over.

ailisp · 2022-09-20T03:25:14Z

src/near-bindgen.ts

+//   target: AnyObject,
+//   key: string | symbol,
+//   descriptor: TypedPropertyDescriptor<Function>
+// ) => void;


Remove or apply these types to functions below?

@ailisp Forgot to remove the comments... I'll need to test out proper typing for these functions.
BTW, do these functions have to actually return an empty function, or anything at all. I want to understand this better so I can actually define proper types.

ailisp · 2022-09-20T03:28:46Z

src/promise.ts

+      this.args,
+      this.amount,
+      this.gas
+    );


My personal feel is above one line isn't too long to be unreadable, but this become kind of lengthy. What is the print width we had for other NEAR JS projects?

@ailisp I haven't set any of those preferences myself, just used prettier defaults, so we can change this to anything really.

ailisp

Thanks for the huge cleaning up, refactoring, formatting and lint works! Besides the above smaller points, I think it's particular important to add the format / lint check to CI, should be added in this PR to avoid future changes that breaks the formatting.

@volovyks Do you aware / have certain preference on the eslintrc?

Change _let_ declarations to _const_ Add whitespace to separate code blocks Refactor code

Fix bullet list formatting Add `includeBytes` import

volovyks · 2022-09-20T11:11:51Z

@petarvujovic98 usually when linting is available - we add it a separate job in GitHub actions. Do you think we should do that?

petarvujovic98 · 2022-09-20T11:13:52Z

@volovyks I think we should. I would first like to properly configure ESLint for this project as it seems that the JavaScript files in the tests and examples are getting false errors about not having experimentalDecorators turned on, when they definitely do.

Change `string` to `Bytes` Format the `Env` interface for more readability

volovyks

@petarvujovic98 it is a great contribution! Let's merge it.
The only thing I would change is to add a CI for linting.
We can exclude tests and examples temporarily and fix the problem in a separate PR. This one is HUGE :) I want to merge it already.

petarvujovic98 · 2022-09-20T11:24:54Z

@volovyks Let me add the CI for linting real quick and let's merge it.

petarvujovic98 · 2022-09-20T11:31:17Z

@volovyks @ailisp Should be ready to merge now.

petarvujovic98 · 2022-09-20T11:37:43Z

@volovyks Do we want to add formatting checks into the Lint workflow as well?

volovyks · 2022-09-20T11:40:19Z

@petarvujovic98 We do not have it in other projects, but let's try it. We can always turn it off.

petarvujovic98 · 2022-09-20T11:42:16Z

@volovyks Done!

Add ignore comments to empty functions Add CLI files to top level `jsconfig.json` Add top level `jsconfig.json` to ESLint config Add `counter.js` example to examples `jsconfig.json`

Ignore `deps` and `build` folders for prettier Build `near-bindgen.ts` with new comments

volovyks · 2022-09-20T12:41:52Z

Merging, great work! @petarvujovic98

ailisp · 2022-09-21T01:19:12Z

Great job! Thanks @petarvujovic98 !

petarvujovic98 requested review from ailisp and volovyks as code owners September 9, 2022 14:05

petarvujovic98 added 7 commits September 19, 2022 13:53

chore: Add ESLint and Prettier

e8093da

Add ESLint and Prettier packages and configs Run ESLint and Prettier Add ESLint and Prettier scripts

chore: Update ESLint configuration

cedd904

Change overrides in ESLint

feat: Update utils

e2639a7

Add includeBytes function declaration Add helper for returning value from collection methods Add helper types Refactor code

feat: Refactor collections

95fb5ad

Add defaultValue to methods that return values Add better type definitions Refactor code

feat: Refactor high level promises

b769d5f

Rename variables and parameters to JavaScript standards Refactor code

feat: Refactor API internally

962335b

Improve type definitions for QJS environment Rename variables and parameters to JavaScript standards Refactor code

feat: Refactor build tools

dba601b

Improve readability of code Add comments for easier understanding Marginaly improve decorator type definitions

petarvujovic98 force-pushed the develop branch from 68d2e8b to dba601b Compare September 19, 2022 20:45

ailisp reviewed Sep 20, 2022

View reviewed changes

src/api.ts

`Promise result ${

status == PromiseResult.Failed

? "Failed"

: status == PromiseResult.NotReady

Copy link

Member

ailisp Sep 20, 2022

Choose a reason for hiding this comment

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

Nice refactor!

ailisp reviewed Sep 20, 2022

View reviewed changes

ailisp approved these changes Sep 20, 2022

View reviewed changes

ailisp mentioned this pull request Sep 20, 2022

Make PromiseIndex type to avoid misuse of promise indices #118

Closed

ailisp mentioned this pull request Sep 20, 2022

Add formatting/linting to project #110

Closed

petarvujovic98 added 2 commits September 20, 2022 12:59

feat: Refactor CLI tools

0a9c285

Change _let_ declarations to _const_ Add whitespace to separate code blocks Refactor code

chore: Fix formatting

16b3f4d

Fix bullet list formatting Add `includeBytes` import

petarvujovic98 added 2 commits September 20, 2022 13:15

fix: Fix parameter types

1f2af4b

Change `string` to `Bytes` Format the `Env` interface for more readability

fix: Add build for API refactor

bfde857

volovyks approved these changes Sep 20, 2022

View reviewed changes

chore: Add linting job to CI

ca1d1d4

chore: Add formatting checks to Lint workflow

58f0966

volovyks assigned petarvujovic98 Sep 20, 2022

volovyks linked an issue Sep 20, 2022 that may be closed by this pull request

Add formatting/linting to project #110

Closed

petarvujovic98 added 2 commits September 20, 2022 14:16

fix: Fix linting errors and ESLint config

e0c4ceb

Add ignore comments to empty functions Add CLI files to top level `jsconfig.json` Add top level `jsconfig.json` to ESLint config Add `counter.js` example to examples `jsconfig.json`

chore: Fix linting and formatting

89678ea

Ignore `deps` and `build` folders for prettier Build `near-bindgen.ts` with new comments

volovyks merged commit 06c9ed6 into near:develop Sep 20, 2022

petarvujovic98 mentioned this pull request Sep 20, 2022

Add option for returning a default value in Map.get #170

Closed

petarvujovic98 mentioned this pull request Sep 22, 2022

feat: Add BigInt serialization #228

Merged

ailisp mentioned this pull request Sep 23, 2022

Collection nesting #202

Closed

Refactor the project and add generics and getter options #213

Refactor the project and add generics and getter options #213

Conversation

petarvujovic98 commented Sep 9, 2022

ailisp commented Sep 13, 2022

ailisp commented Sep 13, 2022

petarvujovic98 commented Sep 13, 2022

ailisp commented Sep 13, 2022

volovyks commented Sep 13, 2022

petarvujovic98 commented Sep 13, 2022

ailisp commented Sep 15, 2022

petarvujovic98 commented Sep 15, 2022

petarvujovic98 commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailisp Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailisp left a comment

Choose a reason for hiding this comment

volovyks commented Sep 20, 2022

petarvujovic98 commented Sep 20, 2022

volovyks left a comment

Choose a reason for hiding this comment

petarvujovic98 commented Sep 20, 2022

petarvujovic98 commented Sep 20, 2022

petarvujovic98 commented Sep 20, 2022

volovyks commented Sep 20, 2022

petarvujovic98 commented Sep 20, 2022

volovyks commented Sep 20, 2022

ailisp commented Sep 21, 2022

ailisp Sep 20, 2022 •

edited

Loading