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

JS Testing Feature: Multiple return values from interaction - Milestone 1 #91

Merged

Conversation

hichana
Copy link
Contributor

@hichana hichana commented Oct 29, 2021

JS Testing Feature: Multiple return values from interaction - Milestone 1

Description

This PR is for issue #30 .

Thank you for reviewing our final submission for "JS Testing Feature: Multiple return values from interaction" FLIP #30. Our solution features:

  • a tuple as the return type from sendTransaction and executeTransaction, which also provides error messaging to other functions in the library that implement them.
  • refactored library internal methods to facilitate the new return types, including jest helper functions
  • refactored tests
  • updated all relevant documentation, including CHANGLOG.md

Initially, our solution was a non-breaking change. We did this by adding a new section in 'flow.json' that could be set to true or false by the developer to decide on whether they wanted to use the new return types or not. There was also a way to pass a bool to the initialization for this same purpose. Internally, we then grabbed the configuration boolean and conditionally returned the old or new functions as per the user's preference. Ultimately we decided that it wasn't beneficial for the users or maintainers of the library for the following reasons:

  • maintainers would have to manage either two sets of tests, or one set with conditional logic based on whether the user has chosen the new return types or the old ones. Once the old functions were finally deprecated completely the maintainer would then need to update all of the tests.
  • maintainer would need to update flow.json, config() and the jest helpers as well once the change has been deprecated
  • maintainer would need to update the documentation more than necessary with simply having a single breaking change
  • users of the library might need to grasp more usage instructions from the documentation to understand the differences between the old and new way of using the functionality
  • if maintainers chose to require it, at some point in the future, legacy users of the library would need to upgrade to the new methods anyways
  • because the new approach is a relatively simple change to the codebase, we believe it's not an unreasonable trouble to place on users of the library. Our updates to the documentation, along with notes we've added to CHANGELOG.md. If our solution is chosen as the winner we recommend copying this to the 'Releases' section (https://github.com/onflow/flow-js-testing/releases) and/or putting it in the README.md of the library

Submission Links & Documents

  • Our code submission has been made to the flow-js-test library as PR #57

Requirements Check

  • Have have you met the milestone requirements? Yes
  • Have you included tests (if applicable)? Yes
  • Have you met the contribution guidelines of the repos you have submitted code to (if applicable)? Yes
  • If this is the last milestone:
    • Demonstrate that you've met all the acceptance criteria (link to code, demos, instructions to run etc.) Yes
    • Demonstrate that you've met all milestone requirements and highlight any extensions or additional work done. Yes
    • Include a payout structure by percentage for each team member (ie. Bob: 20%, Alice: 80%). @J00LZ: 50%, @hichana 50%

Other Details

  • We are happy to review and update with feedback.

@MaxStalker
Copy link
Contributor

おめでとう, @hichana !=^____^=
🎉 Milestone Achieved 🎉

@kerrywei
Copy link

kerrywei commented Nov 5, 2021

nice. merging in this PR

@kerrywei kerrywei merged commit 007e4ac into onflow:main Nov 5, 2021
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.

3 participants