Update dependencies manually #62

joewagner · 2023-02-06T16:47:20Z

This will update the @tableland dependencies and combine all outstanding dependabot pull requests.

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.49.0 to 5.50.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.50.0/packages/parser) --- updated-dependencies: - dependency-name: "@typescript-eslint/parser" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 18.11.18 to 18.11.19. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.9.4 to 4.9.5. - [Release notes](https://github.com/Microsoft/TypeScript/releases) - [Commits](microsoft/TypeScript@v4.9.4...v4.9.5) --- updated-dependencies: - dependency-name: typescript dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.48.2 to 5.50.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.50.0/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [eslint-config-standard-with-typescript](https://github.com/standard/eslint-config-standard-with-typescript) from 33.0.0 to 34.0.0. - [Release notes](https://github.com/standard/eslint-config-standard-with-typescript/releases) - [Changelog](https://github.com/standard/eslint-config-standard-with-typescript/blob/master/CHANGELOG.md) - [Commits](mightyiam/eslint-config-love@v33.0.0...v34.0.0) --- updated-dependencies: - dependency-name: eslint-config-standard-with-typescript dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>

…into combine-prs-branch

…de-18.11.19' into joe/deps

…pt-eslint/parser-5.50.0' into joe/deps

…onfig-standard-with-typescript-34.0.0' into joe/deps

joewagner

@carsonfarmer I'm wondering if you have any ideas on how the tests here could be made to match typescript best practices? I left some inline comments highlighting the issue I'm having.

joewagner · 2023-02-06T17:36:20Z

test/index.test.ts

-    const txnReceipt = await sdk.receipt(hash);
+    const { meta } = await db
+      .prepare("insert into my_table_31337_2 values (1);")
+      .all();


While rewriting this test to work with the new major versions of tableland I found what might be a pain point, and an opportunity to come up with a best practice.
see inline comments below...

joewagner · 2023-02-06T17:42:36Z

test/index.test.ts

+      .all();
+
+    strictEqual(typeof meta.txn?.transactionHash, "string");
+    strictEqual(typeof meta.txn?.chainId, "number");


Because the type returned by await db.prepare(...).all() is unknown the developer should be checking that the javascript object returned matches their expectations. However in the case of writing tests using a strict equal test as above doesn't satisfy the typescript transpiler.

We could potentially do

if (typeof meta.txn?.transactionHash !== "string") throw new Error("test fails but in a weird way!"); if (typeof meta.txn?.chainId !== "number") throw new Error("test fails but in a weird way!");

This satisfies typescript, but might be a bit strange since the errors will never be thrown and tests would normally use an assertion for this kind of thing.

The return type is unknown because we aren't typing it. But the prepare method is generic, so if you specify a type there, then it should be what we expect it to be. If this is not the case, then something is up? Or am I not understanding the question right?

I think I'm just not understanding how to use typescript correctly in this case.
To put my issue another way:
meta.txn?.transactionHash has a type of 'string | undefined' def
and the validator.receiptByTransactionHash method expects an argument object with a transactionHash key that is string. I tried specifying the generic type for the prepare method, but I couldn't get that to work.
The two ways I've been able to coerce the type string | undefined to string is either by using a typeof conditional, or an as type assertion. (These both work, just wondering if there's a better way?)

Ah ok I see. Its type is actually derived here: https://github.com/tablelandnetwork/js-tableland/blob/main/src/validator/receipt.ts#L26. And transactionHash is actually a string. The reason you are getting string | undefined is because you are using the conditional chaining syntax: meta.txn?.transactionHash (note the question mark). The way to deal with this in typescript is to assert that meta.txn is not undefined. And then everything will show up as their correct type.

There is no way for us know know for sure ahead of time if meta.txn is defined. But you can do:

if (meta.txn == null) { // throw or something } // then things will work here

I think that makes sense. Just to make sure I understand, I'll push a new commit with one test using as and the other test using throw

You can also use asserts, and typescript should be smart enough to deal with that correct. So you can so something like (or similar) and the type checking should know that any check after this is going to be safe.

strictNotEquals(meta.txn, null);

joewagner · 2023-02-06T17:44:08Z

test/index.test.ts

+      .all();
+    const txnReceipt = await validator.receiptByTransactionHash({
+      transactionHash: meta.txn?.transactionHash as string,
+      chainId: meta.txn?.chainId as number,


using the as type assertion or a ts-ignore comment does work, but is potentially not good practice. It would be good if these tests could serve as example for best practices.

Yeh that shouldn't be required. Did something funny happen with the latest release in terms of types?

carsonfarmer · 2023-02-06T18:29:33Z

test/index.test.ts

-    const txnReceipt = await sdk.receipt(hash);
+    const { meta } = await db
+      .prepare("insert into my_table_31337_2 values (1);")
+      .all();


You shouldn't be using all here for writes. It is much better to use the run method and get the proper type back.

Good to know, I'll update that.

joewagner · 2023-02-06T20:05:25Z

test/index.test.ts

+      .run();
+
+    if (typeof meta.txn?.transactionHash !== "string") {
+      throw new Error("something unexpected happened during insert");


This will never be thrown, but we have to put a check here since typescript will not build because of type mismatch

Correct. But it isn't throwing because of a type mismatch per se. It is just trying to protect us from doing something with something that might be undefined. So we just have to use type guards like this to be safe.

joewagner · 2023-02-06T20:07:27Z

test/index.test.ts

+    strictEqual(typeof meta.txn?.chainId, "number");
+
+    const txnReceipt = await validator.receiptByTransactionHash({
+      transactionHash: meta.txn?.transactionHash as string,


We have to use as here in order for typescript to build, even though this will never be executed if transactionHash is undefined because of the strictEqual check above.

Well we can use as here. But you can also use assertions in tests because we know it is going to be valid. Something like this could work:

Suggested change

transactionHash: meta.txn?.transactionHash as string,

transactionHash: meta.txn!.transactionHash,

Awesome! this is much nicer to look at and we still get to keep our assertions above. I'll update the other test

Spoke too soon here. This is not allowed by our linter because of @typescript-eslint/no-non-null-assertion

I added an eslint-disable for the failing rule. Maybe that combined with the ! assertion is the best option?

joewagner · 2023-02-06T20:08:59Z

@carsonfarmer As a double check, we should use the pattern in the "insert" test instead of the one in the "update" test?

awmuncy · 2023-04-20T16:37:08Z

@joewagner This PR has been sitting for a while. Does it need updates, or is it RFR?

joewagner · 2023-04-20T16:40:02Z

@joewagner This PR has been sitting for a while. Does it need updates, or is it RFR?

It was ready, but looks like it's out of date now. I'll revisit it today.

joewagner · 2023-04-20T17:59:29Z

@awmuncy Ok, this is updated with the latest dependabot prs and is RFR

awmuncy · 2023-04-20T19:46:42Z

package.json

  },
  "dependencies": {
-    "@tableland/sdk": "^3.1.7"
+    "@tableland/sdk": "^4.0.0-pre.7"


Could we update this to 4.1.0+?

Definitely! Not sure how I missed that.

awmuncy

LGTM!

dependabot bot and others added 12 commits February 6, 2023 13:17

Merge dependabot/npm_and_yarn/typescript-eslint/eslint-plugin-5.50.0 …

035d738

…into combine-prs-branch

Merge dependabot/npm_and_yarn/typescript-4.9.5 into combine-prs-branch

2548349

Update tableland packages

364a6b7

Merge remote-tracking branch 'origin/combine-prs-branch' into joe/deps

74526d5

Merge remote-tracking branch 'origin/dependabot/npm_and_yarn/types/no…

3fc7eff

…de-18.11.19' into joe/deps

Merge remote-tracking branch 'origin/dependabot/npm_and_yarn/typescri…

63afc4b

…pt-eslint/parser-5.50.0' into joe/deps

Merge remote-tracking branch 'origin/dependabot/npm_and_yarn/eslint-c…

64c5c59

…onfig-standard-with-typescript-34.0.0' into joe/deps

joewagner requested a review from carsonfarmer as a code owner February 6, 2023 16:47

joewagner added 2 commits February 6, 2023 09:52

bump local tableland pre-release version

1b7bd2a

wip update tests for new tableland package version

431b14f

joewagner commented Feb 6, 2023

View reviewed changes

carsonfarmer reviewed Feb 6, 2023

View reviewed changes

wip: updates tests to use different ts patterns

d2bbdd2

joewagner commented Feb 6, 2023

View reviewed changes

joewagner added 2 commits February 6, 2023 15:20

disable lint rule for tests

ed3882b

Test cleanup

b936140

joewagner marked this pull request as draft April 20, 2023 16:40

joewagner added 2 commits April 20, 2023 11:47

Fix ts errors in tests

9c922ac

conbine existing dependabot prs

88dc79b

joewagner marked this pull request as ready for review April 20, 2023 17:58

joewagner requested a review from awmuncy April 20, 2023 17:58

joewagner requested a review from carsonfarmer April 20, 2023 18:15

awmuncy reviewed Apr 20, 2023

View reviewed changes

Bump @tableland deps

4696b6d

awmuncy approved these changes Apr 20, 2023

View reviewed changes

joewagner changed the base branch from main to staging April 20, 2023 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update dependencies manually #62

Update dependencies manually #62

joewagner commented Feb 6, 2023

joewagner left a comment

joewagner Feb 6, 2023

joewagner Feb 6, 2023

joewagner Feb 6, 2023

carsonfarmer Feb 6, 2023

joewagner Feb 6, 2023

carsonfarmer Feb 6, 2023

joewagner Feb 6, 2023

carsonfarmer Feb 6, 2023 •

edited

Loading

joewagner Feb 6, 2023

carsonfarmer Feb 6, 2023

carsonfarmer Feb 6, 2023

joewagner Feb 6, 2023

joewagner Feb 6, 2023

carsonfarmer Feb 6, 2023

joewagner Feb 6, 2023

carsonfarmer Feb 6, 2023 •

edited

Loading

joewagner Feb 6, 2023

joewagner Feb 6, 2023

joewagner Feb 6, 2023

joewagner commented Feb 6, 2023

awmuncy commented Apr 20, 2023

joewagner commented Apr 20, 2023

joewagner commented Apr 20, 2023

awmuncy Apr 20, 2023

joewagner Apr 20, 2023

awmuncy left a comment

	transactionHash: meta.txn?.transactionHash as string,
	transactionHash: meta.txn!.transactionHash,

Update dependencies manually #62

Are you sure you want to change the base?

Update dependencies manually #62

Conversation

joewagner commented Feb 6, 2023

joewagner left a 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

carsonfarmer Feb 6, 2023 • 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

carsonfarmer Feb 6, 2023 • 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

joewagner commented Feb 6, 2023

awmuncy commented Apr 20, 2023

joewagner commented Apr 20, 2023

joewagner commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awmuncy left a comment

Choose a reason for hiding this comment

carsonfarmer Feb 6, 2023 •

edited

Loading

carsonfarmer Feb 6, 2023 •

edited

Loading