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

Update viewFunction with only accepting single object as argument #935

Merged
merged 5 commits into from
Oct 4, 2022

Conversation

hcho112
Copy link
Contributor

@hcho112 hcho112 commented Aug 22, 2022

Motivation

This PR is removing old syntax that is expected to be deprecated. Since this PR contains BREAKING CHANGE, prior to merge, please follow up with team to confirm correct schedule.

Description

This PR will update viewFunction in account.ts to only support single object as argument. (Deprecating on supporting old convention of accepting array of arguments)
Also updates existing tests

Checklist

  • Read the contributing guidelines
  • Commit messages follow the conventional commits spec
  • Performed a self-review of the PR
  • Added automated tests
  • Manually tested the change

@hcho112 hcho112 requested a review from MaximusHaximus August 22, 2022 03:33
Copy link
Contributor

@morgsmccauley morgsmccauley left a comment

Choose a reason for hiding this comment

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

Looks good, just one comment

@@ -483,40 +483,17 @@ export class Account {
* @param contractId NEAR account where the contract is deployed
Copy link
Contributor

Choose a reason for hiding this comment

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

This JSDoc is configured for separate arguments, now that viewFunction expects a single object argument I believe we need something similar to the previous options - https://jsdoc.app/tags-param.html

morgsmccauley
morgsmccauley previously approved these changes Aug 31, 2022
@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2022

🦋 Changeset detected

Latest commit: c2f93a9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
near-api-js Major
@near-js/cookbook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@morgsmccauley morgsmccauley left a comment

Choose a reason for hiding this comment

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

Just a couple comments around the included Changeset.

Also, because this is a major bump, let's hold off on merging this. It might be better to publish the minor/patch changes first so that they aren't lumped in to the major change. That would give devs more granularity in terms of upgrading.

@@ -0,0 +1,6 @@
---
"@near-js/cookbook": minor
Copy link
Contributor

Choose a reason for hiding this comment

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

🔪

"near-api-js": major
---

Update `viewFunction` on account model to only accept single object as argument
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is a breaking change it would pay to add more detail here, including migration info. What do you think of the following:

account.viewFunction now takes a single object argument rather than individual args. Callsites will now need to be updated like so:

-await account.viewFunction(
-  'wrap.near',
-  'storage_balance_of',
-  { account_id: 'example.near' }
-);
+await account.viewFunction({
+  contractId: 'wrap.near',
+  methodName: 'storage_balance_of',
+  args: { account_id: 'example.near' },
+});

morgsmccauley
morgsmccauley previously approved these changes Oct 4, 2022
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