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

Remove substrate-compat default feature flag #1078

Merged
merged 10 commits into from
Jul 21, 2023

Conversation

tadeohepperle
Copy link
Contributor

fixes #1077

I added target_arch = "wasm32" to the jsonrpsee_helpers mod because otherwise jsonrpsee does not expose the client_transport::web module.

@tadeohepperle tadeohepperle requested review from a team as code owners July 20, 2023 12:34
Comment on lines 278 to 279
wasm-pack test --no-default-features --features jsonrpsee,web --headless --firefox
wasm-pack test --no-default-features --features jsonrpsee,web --headless --chrome
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be unnecessary shouldn't they? The wasm-rpc-tests crate doesn't expose any features, and so this should have no effect.

Comment on lines 80 to 81
//! #[cfg(feature = "substrate-compat")]
//! {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ooh, putting the feature on a block is much cleaner than my suggesiton; good idea!

I'd suggest hiding these lines though (with a # prefix) and not indenting the rest, since (a) most of it doesn't require that feature and (b) we talk about needing it above anyway, so keeping the example code neater feels better :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tadeohepperle this is the only thing left that I'd like to see done :)

@@ -468,7 +468,7 @@ mod jsonrpsee_helpers {
}

// helpers for a jsonrpsee specific OnlineClient.
#[cfg(all(feature = "jsonrpsee", feature = "web"))]
#[cfg(all(feature = "jsonrpsee", feature = "web", target_arch = "wasm32"))]
Copy link
Collaborator

@jsdw jsdw Jul 20, 2023

Choose a reason for hiding this comment

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

It might be a slightly better user experience to throw a compile_error if the target isn't wasm32 and jsonrpsee expects that, eg:

#[cfg(all(feature = "jsonrpsee", feature = "web"))]
mod jsonrpsee_helpers {
    #[cfg(not(target_arch = "wasm32")]
    compile_error!("when using jsonrpsee and web features, the target arch must be wasm32");

    // ...
}

So that users know what the issue is if things don't compile properly with these features.

It also self documents for future us why that target_arch is there :)

@@ -17,7 +17,7 @@ keywords = ["parity", "substrate", "blockchain"]
[features]
# For dev and documentation reasons we enable more features than are often desired.
# it's recommended to use `--no-default-features` and then select what you need.
default = ["jsonrpsee", "native", "substrate-compat"]
default = ["jsonrpsee", "native"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@jsdw
Copy link
Collaborator

jsdw commented Jul 20, 2023

For the doc errors:

error: unresolved link to `sp_core::Pair`
   --> subxt/src/book/usage/transactions.rs:5:1
    |
5   | / //! # Transactions
6   | | //!
7   | | //! A transaction is an extrinsic that's signed (ie it originates from a given address). The purpose
8   | | //! of extrinsics is to modify the node storage in a deterministic way, and so being able to submit
...   |
210 | | //! [`crate::tx::TxInBlock`] for more options.
211 | | //!
    | |___^
    |
    = note: the link appears in this line:
            
            2. Alternately, Subxt can use instances of Substrate's [`sp_core::Pair`] to sign things by wrapping
                                                                    ^^^^^^^^^^^^^^^
    = note: no item named `sp_core` in scope
    = note: requested on the command line with `-D rustdoc::broken-intra-doc-links`

error: unresolved link to `crate::tx::PairSigner`
   --> subxt/src/book/usage/transactions.rs:5:1
    |
5   | / //! # Transactions
6   | | //!
7   | | //! A transaction is an extrinsic that's signed (ie it originates from a given address). The purpose
8   | | //! of extrinsics is to modify the node storage in a deterministic way, and so being able to submit
...   |
210 | | //! [`crate::tx::TxInBlock`] for more options.
211 | | //!
    | |___^
    |
    = note: the link appears in this line:
            
            them in a [`crate::tx::PairSigner`] (requires the `substrate-compat` feature to be enabled).
                       ^^^^^^^^^^^^^^^^^^^^^^^
    = note: no item named `PairSigner` in module `tx`

error: could not document `subxt`

I guess the best solution is just to remove the [/]'s to stop them being doc links, so the feature isn't required for those docs. We can prolly use cfg flags and such to work around it but it'd get messy!

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jsdw jsdw requested a review from lexnv July 21, 2023 15:31
Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM!

@jsdw jsdw merged commit 6c8d878 into master Jul 21, 2023
@jsdw jsdw deleted the remove-substrate-compat-default-deature-flag branch July 21, 2023 17:10
@jsdw jsdw mentioned this pull request Jul 24, 2023
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.

Remove "substrate-compat" default feature flag
3 participants