Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

Add CurrentProvider Type Definition in index.d.ts #710

Merged
merged 4 commits into from
Oct 25, 2022

Conversation

godtaehee
Copy link
Contributor

Proposed changes

  • Describe your changes to communicate to the maintainers why we should accept this pull request.
  • If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

  • Please leave the issue numbers or links related to this PR here.

Further comments

I wrote all of my issue in this issue

and this is my repository which prove my inconvenient about caver.js

Thanks.

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@github-actions
Copy link

github-actions bot commented Oct 20, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@jimni1222
Copy link
Contributor

@godtaehee Thank you for contribution. Please sign CLA. #710 (comment)

types/index.d.ts Outdated Show resolved Hide resolved
@kjhman21
Copy link
Contributor

@godtaehee Could you please also add a simple test case to check whether this is accessible from the client code?

@godtaehee
Copy link
Contributor Author

godtaehee commented Oct 21, 2022

@godtaehee Thank you for contribution. Please sign CLA. #710 (comment)

Thanks, I just signed to CLA

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@jimni1222 jimni1222 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for contribution!!
After resolving @kjhman21 comment, we can merge this and release :)

@godtaehee
Copy link
Contributor Author

@godtaehee Could you please also add a simple test case to check whether this is accessible from the client code?

Sure, sir. I will make some test case, and push it!

@jimni1222
Copy link
Contributor

@godtaehee Thank you for contribution. Please sign CLA. #710 (comment)

Thanks, I just signed to CLA

I have read the CLA Document and I hereby sign the CLA

I think you need to write I have read the CLA Document and I hereby sign the CLA comment separate ;-; Still https://github.com/klaytn/caver-js/actions/runs/3293913833/jobs/5430893789 says failed to check

@godtaehee
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@godtaehee
Copy link
Contributor Author

@kjhman21 I just made test code in types/test/caver-test.ts

I can use currentProvider when I use it in js environment using require like my repository code

So I think there is no need for testing in js file which is located in root/test/[all of test file].js

I searched all of test code in repository. but I can't find the location where I test it.

So If this my changes is not sufficient for testing my PR, Please give me Advice!

Sorry for not well in contributing.

Copy link
Contributor

@kjhman21 kjhman21 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@godtaehee
Copy link
Contributor Author

godtaehee commented Oct 24, 2022

Thanks :D I will nestjs-caver which can be interacted with nest.js and caver!

@kjhman21 kjhman21 merged commit 6f0d102 into klaytn:dev Oct 25, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants