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

updated rxjs version from 6 to 7, ts to 4.2.4, and some refactor #2535

Merged
merged 9 commits into from
Jun 29, 2023

Conversation

dsawali
Copy link
Contributor

@dsawali dsawali commented Jun 19, 2023

closes #2261

  • updated RxJS version from v6.x to v7.8.1
  • updated TS version for the @taquito/taquito package (and the parent package) from 4.1.5 to 4.2.4
  • refactored some objects to have typechecks
  • added a type to encompass some typechecks better
  • refactored some RxJS 6 methods that were deprecated
  • added ts-ignore to circumvent TS incompatibility with michel-codec (will be removed in the future in a refactor)
  • updated some unit tests to comply with additional typechecks

Thank you for your contribution to Taquito.

Before submitting this PR, please make sure:

  • Your code builds cleanly without any errors or warnings
  • You have run the linter against the changes
  • You have added unit tests (if relevant/appropriate)
  • You have added integration tests (if relevant/appropriate)
  • All public methods or types have TypeDoc coverage with a complete description, and ideally an @example
  • You have added or updated corresponding documentation
  • If relevant, you have written a first draft summary describing the change for inclusion in Release Notes.

In this PR, please also make sure:

  • You have linked this PR to the issue by putting closes #TICKETNUMBER in the description box (when applicable)
  • You have added a concise description on your changes

Release Note Draft Snippet

If relevant, please write a summary of your change that will be suitable for
inclusion in the Release Notes for the next Taquito release.

@netlify
Copy link

netlify bot commented Jun 19, 2023

Deploy Preview for taquito-test-dapp ready!

Name Link
🔨 Latest commit 4840f1a
🔍 Latest deploy log https://app.netlify.com/sites/taquito-test-dapp/deploys/649dd676888d3f00081cad28
😎 Deploy Preview https://deploy-preview-2535--taquito-test-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link

github-actions bot commented Jun 19, 2023

New packages have been deployed to the preview repository at https://npm.preview.tezostaquito.io/.

Published packages:

npm i @taquito/core@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/http-utils@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/contracts-library@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/local-forging@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/tzip12@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/signer@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/remote-signer@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/rpc@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/tzip16@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/utils@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/michelson-encoder@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/ledger-signer@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/michel-codec@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/sapling@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/taquito@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/
npm i @taquito/beacon-wallet@17.0.0-9e5b818-- --registry https://npm.preview.tezostaquito.io/

@github-actions
Copy link

github-actions bot commented Jun 19, 2023

A new deploy preview is available on Netlify at https://9e5b818--tezostaquito.netlify.app

@dsawali dsawali marked this pull request as ready for review June 27, 2023 23:48
@@ -58,7 +59,8 @@ export class BatchOperation
.filter((result) => BATCH_KINDS.indexOf(result.kind) !== -1)
.map((result) => {
if (hasMetadataWithResult(result)) {
return result.metadata.operation_result.status;
const opResult = result.metadata.operation_result as BatchOperationResult;
Copy link
Collaborator

@hui-an-yang hui-an-yang Jun 28, 2023

Choose a reason for hiding this comment

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

is it better to defined opResult as type BatchOperationResult then to cast it?
const opResult: BatchOperationResult = result.metadata.operation_result;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

casting is necessary still since opResult is type unknown. TS will also throw an error if you only type it as such. Same comment as the other one

Copy link
Collaborator

@hui-an-yang hui-an-yang Jun 28, 2023

Choose a reason for hiding this comment

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

the operation_result type seems lost in hasMetadataWithResult, will we consider defined it better in the function so we can avoid casting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS unfortunately doesn't derive the subset of types from hasMetadataWithResult. It will always return unknown regardless, which is the reason why we needed to cast it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand :)

@@ -130,7 +135,7 @@ export class Operation {
return (
this.results.map((result) => {
if (hasMetadataWithResult(result)) {
return result.metadata.operation_result.status;
return (result.metadata.operation_result as OperationResult).status;
Copy link
Collaborator

Choose a reason for hiding this comment

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

preference wise, i'd declare type defined variable and return variable.status then casting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment as the one above

Copy link
Collaborator

@hui-an-yang hui-an-yang left a comment

Choose a reason for hiding this comment

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

🌟 Great work to migration the code base to rxjs 7 and ts 4.2.4.
Besides type casting that I had impression that it compromise type safety, was advised try best to avoid.
Just some minor comments, the rest lgtm 🙌

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for updating example!!

Copy link
Collaborator

@hui-an-yang hui-an-yang left a comment

Choose a reason for hiding this comment

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

lgtm🎉

@dsawali dsawali merged commit a763b7b into master Jun 29, 2023
11 of 12 checks passed
@dsawali dsawali deleted the 2261-rxjs branch June 29, 2023 20:20
dvkam pushed a commit to dvkam/taquito that referenced this pull request Jul 6, 2023
…dlabs#2535)

* updated rxjs version from 6 to 7, ts to 4.2.4, and some necessary refactors

* updated unit tests, and refactored necessary packages

* updated example

* addressed PR comments

* debug tests

* debug flextesa failure

* remove debug statements

* trigger build

* try previous flextesa version
hui-an-yang pushed a commit that referenced this pull request Jul 11, 2023
* fix issues from #1652

* added TZIP link above the column caption

* removed the TZIP link at the bottom of the page

* updated rxjs version from 6 to 7, ts to 4.2.4, and some refactor (#2535)

* updated rxjs version from 6 to 7, ts to 4.2.4, and some necessary refactors

* updated unit tests, and refactored necessary packages

* updated example

* addressed PR comments

* debug tests

* debug flextesa failure

* remove debug statements

* trigger build

* try previous flextesa version

* fix issues from #1652

* changed color back to brown like it was before

---------

Co-authored-by: Davis Sawali <davis.sawali@ecadlabs.com>
Co-authored-by: David <david.kaminski93@gmail.com>
hui-an-yang added a commit that referenced this pull request Jul 17, 2023
* Fix issue #1652 (#2547)

* fix issues from #1652

* added TZIP link above the column caption

* removed the TZIP link at the bottom of the page

* updated rxjs version from 6 to 7, ts to 4.2.4, and some refactor (#2535)

* updated rxjs version from 6 to 7, ts to 4.2.4, and some necessary refactors

* updated unit tests, and refactored necessary packages

* updated example

* addressed PR comments

* debug tests

* debug flextesa failure

* remove debug statements

* trigger build

* try previous flextesa version

* fix issues from #1652

* changed color back to brown like it was before

---------

Co-authored-by: Davis Sawali <davis.sawali@ecadlabs.com>
Co-authored-by: David <david.kaminski93@gmail.com>

* docs: extended website adjustment changes to next version

* revert: package-lock.json to comply with master

* docs: removed broken link in lambda_view and update sapling contract for live code example

* docs: updated version 17.1.0 doc changes accordingly

---------

Co-authored-by: dvkam <82422481+dvkam@users.noreply.github.com>
Co-authored-by: Davis Sawali <davis.sawali@ecadlabs.com>
Co-authored-by: David <david.kaminski93@gmail.com>
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.

Upgrading to rxjs version 7.8.1
2 participants