Skip to content

[types] Remove unnecessary deps #261

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

Merged
merged 1 commit into from
Apr 13, 2021
Merged

[types] Remove unnecessary deps #261

merged 1 commit into from
Apr 13, 2021

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Apr 2, 2021

No description provided.

@c410-f3r c410-f3r force-pushed the stuff branch 3 times, most recently from 4cb5a48 to 120730e Compare April 3, 2021 00:03
Copy link
Contributor

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.

Looks overall good but I don't really understand the rationale behind making this crate no-std without the client traits then it's not super useful, such as the proc macros wouldn't work. I suppose making the SubscriptionClient trait std only (subscription type) and make Client trait no-std would do it.

Can you elaborate why making the types no-std is useful? The intention is that it's a internal crate for jsonrpsee but maybe useful for other purposes?

@niklasad1 niklasad1 requested a review from maciejhirsz April 3, 2021 10:46
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Apr 3, 2021

no_std support is usually a complementary and natural step when removing unnecessary dependencies. Besides, isn't it something desirable in the long run?

If necessary, I can remove no_std stuff. No problem.

@niklasad1
Copy link
Contributor

niklasad1 commented Apr 3, 2021

no_std support is usually a complementary and natural step when removing unnecessary dependencies. Besides, isn't it something desirable in the long run?

Sure, why not it introduces some boiler plate code for the error stuff but worth it if somebody wants to use the types in no-std but we have no plans making client/server to support for no-std however we have plans to move the HTTP client to use surf as backend for WASM support

Copy link
Contributor

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM except the typo, see inline suggestion.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Apr 3, 2021

IIRC, WASM language isn't aware of std stuff (filesystem, threads, timer, etc) and that is one of the reasons for the creation of WASI.

@niklasad1
Copy link
Contributor

niklasad1 commented Apr 3, 2021

IIRC, WASM language isn't aware of std stuff (filesystem, threads, timer, etc) and that is one of the reasons for the creation of WASI.

Yes, but it's possible to do it with JavaScript bindings too as surf/http-client does.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Apr 3, 2021

Cool! Thanks for the link

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Reducing compile times is great, thank you so much for looking into this.

I agree with @niklasad1 that no-std is not a goal and adds more noise than it's worth imo. I also think that the added boilerplate around errors is questionable. I prefer it the way it was.

Any chance we can keep the compile time improvements without the inlining, no-std, and error re-implementations?

@c410-f3r c410-f3r changed the title [types] Remove unnecessary deps and add no_std support [types] Remove unnecessary deps Apr 12, 2021
@niklasad1
Copy link
Contributor

@dvdplm please take another look at this PR ^^

@dvdplm dvdplm merged commit e4c8da5 into paritytech:master Apr 13, 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