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

polymorphic with improved exports #6

Closed
wants to merge 9 commits into from

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Mar 18, 2024

Relates to ubiquity/pay.ubq.fi#205

Original context: ubiquity/pay.ubq.fi#205 (comment)
  • reliance on package.json to serve correct module from @ubiquitydao/rpc-handler
  • added a fallback hook that handles dynamic module export if ^ has issues
  • added rpcTimeout as config option
  • added test-specific build (replacing axios with fetch due to axios adapter issues)
  • added axios back as it's err handling is better than fetch (I swapped it out originally 1. to reduce pkg size and 2. to pass tests)
  • fixed bug with latency naming causing chain-agnostic rpc calls, meaning regardless of network all localStorage latencies would be considered safe (at least was the case for chains 100 & 1) and executed on.
    • local storage is wiped if coming from chain A to chain B

Allowing the data-tracking to be configurable would be a substantial change and would beef the package up quite handsomely because it would require that all RPCs for every chain get injected at build time and then adding a filtering method for returning list of acceptable providers meeting their data-tracking needs

What's the consensus in adding this feature?

Keyrxng added 5 commits March 15, 2024 15:09
- added rpcTimeout as config option
- added test-specific build
- fixed bug with latency naming causing chain-agnostic rpc calls
Copy link
Contributor

github-actions bot commented Mar 18, 2024

Lines Statements Branches Functions
Coverage: NaN%
Unknown% (0/0) Unknown% (0/0) Unknown% (0/0)

JUnit

Tests Skipped Failures Errors Time
0 0 💤 0 ❌ 0 🔥 3.157s ⏱️
Coverage Report (0%)
File% Stmts% Branch% Funcs% LinesUncovered Line #s
All files0000 

src/services/rpc-service.ts Outdated Show resolved Hide resolved
tests/mocks/handler.ts Show resolved Hide resolved
tests/mocks/rpc-handler.ts Outdated Show resolved Hide resolved
tests/mocks/rpc-handler.ts Outdated Show resolved Hide resolved
tests/mocks/rpc-service.ts Show resolved Hide resolved
tests/rpc-handler.test.ts Show resolved Hide resolved
types/constants.ts Outdated Show resolved Hide resolved
types/handler.ts Outdated Show resolved Hide resolved
types/handler.ts Outdated Show resolved Hide resolved
types/rpc-handler.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Mar 18, 2024

Data tracking feature is fine but I'm personally not concerned with it.

Also consider making more precise changes for handling issues. And then opening a separate pull for refactoring and miscellaneous changes.

It feels like searching for a needle in a haystack for the reviewers otherwise and it's difficult to focus on the important changes with a pull of this size.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Mar 18, 2024

Data tracking feature is fine but I'm personally not concerned with it.

Also consider making more precise changes for handling issues. And then opening a separate pull for refactoring and miscellaneous changes.

It feels like searching for a needle in a haystack for the reviewers otherwise and it's difficult to focus on the important changes with a pull of this size.

duly noted! After seeing molecula handle the commit history so clean I thought best to try follow suit but I'm not quite at that level yet eh lmao

I'll refactor optionals to default tmrw but I do believe that the rpcTimeout is a feature that should stay

types/rpc-handler.ts Outdated Show resolved Hide resolved
Co-authored-by: アレクサンダー.eth <[email protected]>
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Apr 23, 2024

Is there anything holding this back except the merge conflict? If there is I'll handle them together

@Keyrxng
Copy link
Contributor Author

Keyrxng commented May 20, 2024

I have split this PR into two, one for tests and one for polymorphic exports.

If it's easier to review the two split then this could be closed and I'll open the other two, unless that would make further review more difficult.

Either way, considering the NPM package is now published and progress can be made in relevant issues it's better to close this as not planned and stick with import {} from "@ubq../dist/esm/src/... or go through with this as-is or through a new PR specific to improving only the exports and using import {} from @ubq/rpc-handler

This was referenced May 20, 2024
@rndquu
Copy link
Member

rndquu commented May 21, 2024

@Keyrxng This PR should be closed in favour of #14 and #15, correct?

@Keyrxng
Copy link
Contributor Author

Keyrxng commented May 21, 2024

That's right @rndquu

@rndquu rndquu closed this May 21, 2024
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