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

Apply context in constructor #5850

Closed
wants to merge 2 commits into from
Closed

Apply context in constructor #5850

wants to merge 2 commits into from

Conversation

kevinam99
Copy link

Description

Aims to fix the issue linked below to apply context in the constructor which was previously not done.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist for 4.x:

  • I have selected the correct 4.x base branch.
  • Within the description, the feature or issue is discussed in detail or an issue is linked.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added any required tests for the changes I made
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged and published in downstream modules.
  • I ran yarn successfully
  • I ran yarn lint successfully
  • I ran yarn build:web successfully
  • I ran yarn test:unit successfully
  • I ran yarn test:integration
  • I ran compile:contracts successfully but it ended up removing important comments, so didn't commit this change
  • I have tested my code.
  • I have updated the corresponding CHANGELOG.md file in the packages I have edited.

@kevinam99
Copy link
Author

I can't seem to add the pleasereview label. Could anyone with permissions please add it?

@spacesailor24 spacesailor24 added Changes Requested Requires further changes 4.x 4.0 related labels Feb 21, 2023
@spacesailor24
Copy link
Contributor

Hi there! Thank you for your interest in contributing to web3.js!

After reviewing the change you made, I've realized that I made a mistake in my understanding of the overloaded constructor: Only these parameter names are to be used within the constructor. This is because TypeScript allows for overloaded constructors, but only allows for a single implementation that is compatible with the other defined function signatures. This is why the second constructor argument is called addressOrOptionsOrContext, because it's a catch-all for all the second arguments mentioned in the function signatures above it

I suspect that the bug causing #5802 is probably because of the first part of this comment. If you'd still like to contribute to web3.js, you could see if this is actually the issue and attempt to fix the bug there. Either way, thank you for your interest in helping us maintain the library

@kevinam99
Copy link
Author

Thank you for the very detailed explanation. Could you help me with some guidance on where or how I could start? Would be nice to contribute to this project since I am a user myself at work and personal projects.

@spacesailor24
Copy link
Contributor

From the comment I linked above,

When Contract is passed an instance of Web3Context, this config is undefined because the expected Web3Context.config is actually _config and Web3Config.getConfig is expected to be used to access the config object

So I'm assuming we'd need to update Web3Context to get _config from providerOrContexthere if providerOrContext is an instanceof Web3Context using getConfig or something

@spacesailor24
Copy link
Contributor

If you're curious @kevinam99, this was a possible fix for this issue

Thank you again for your interest and contributions! Closing this in favor of #5950

@kevinam99
Copy link
Author

Thank you @spacesailor24. I had just gotten some time to work on it, but no problem.

@spacesailor24
Copy link
Contributor

@kevinam99 sorry about that, I'll keep you in mind if any other good-first-issues pop up, but if you'd still like to contribute, testing the latest RC for 4.x in your projects would be a huge help as we're trying to iron out any issues for migrating users

@kevinam99
Copy link
Author

@spacesailor24 sure, thanks :)
I'll check the latest RC like you said and create an issue in case I find something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x 4.0 related Changes Requested Requires further changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants