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

Percent Decode Url Parameters #2251

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

zoomiti
Copy link
Contributor

@zoomiti zoomiti commented Feb 1, 2024

In my application I have some routes with parameters that can be user defined and often include spaces. This means that there usually is a %20 instead of a space in the parameter which is unexpected from a dev perspective. This makes sure that when building the parameter map we percent decode the parameter.

This breaks a doc test and I have a proposed fix for it since it now requires the "ssr" feature to work. The test now only runs when the ssr feature is enabled.

@benwis
Copy link
Contributor

benwis commented Feb 2, 2024

So the question with params matching here is what happens on client side routing. After the initial page hit, which is rendered on the server, all other navigation is done on the client. I'm wondering if that will cause your urls to not decode and cause issues

Did you test that eventuality?

@zoomiti
Copy link
Contributor Author

zoomiti commented Feb 2, 2024

There already exists a client side function for decoding which it should use for client side routing.

Regardless I did just finish testing it on my personal site and client side routing is working just fine.

@benwis
Copy link
Contributor

benwis commented Feb 2, 2024

Can you run cargo clippy and cargo fmt on it? That's what's making it fail the CI

@zoomiti
Copy link
Contributor Author

zoomiti commented Feb 2, 2024

I have, it didn't change anything

@gbj
Copy link
Collaborator

gbj commented Feb 2, 2024

@benwis Clippy is complaining about an issue inside rkyv, here and everywhere. I haven't had time to look into it. These thing pop up sometimes because it's running nightly clippy.

@martinfrances107
Copy link
Contributor

martinfrances107 commented Feb 4, 2024

Clippy is complaining about an issue inside rkyv, here and everywhere.

I am just starting off the bug hunt investigation .. and reporting what I see as I go
Sometimes just talking about it in a reasoned way allow others to effortlessly jump directly to the right solution.

In the build artefacts I see this error

running `cargo clippy --no-default-features --all-features` on leptos (19/120)
error: process didn't exit successfully: `/home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo clippy --manifest-path /home/runner/work/leptos/leptos/leptos/Cargo.toml --no-default-features --all-features` (exit status: 101)

From the main branch, here is what I interpret as the failing command

cargo +nightly clippy --no-default-features --all-features

so we can peek what is behind the (exit status 101).

I get a long list of errors, all of the the same form. So I am just recording the first two errors below

They are rkyv-0.7.43 related

When I look at the full list of errors, looking for a entry point, a place in the leptos code which is a root of all this ... I get no joy...

What do we need to do to get rkyv to compile? in this instance

So I am expecting a the answer to be and complex nested inter-dependency
where a sub module we import has a bug and is not setting the correct feature flags to ensure compilation.

When I use cargo why rkyv or cargo tree to grapple with these issues.
I am doing something wrong as I can I see no dependence on rkyv.... so any advice would be welcome

error[E0277]: the trait bound `NonZero<i8>: ArchiveCopy` is not satisfied
   --> /home/martin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rkyv-0.7.43/src/copy.rs:108:33
    |
108 | unsafe impl ArchiveCopySafe for NonZeroI8 {}
    |                                 ^^^^^^^^^ the trait `ArchiveCopy` is not implemented for `NonZero<i8>`
    |
note: required by a bound in `ArchiveCopySafe`
   --> /home/martin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rkyv-0.7.43/src/copy.rs:99:35
    |
99  | pub unsafe trait ArchiveCopySafe: ArchiveCopy + Sized {}
    |                                   ^^^^^^^^^^^ required by this bound in `ArchiveCopySafe`

error[E0277]: the trait bound `NonZero<u8>: ArchiveCopy` is not satisfied
   --> /home/martin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rkyv-0.7.43/src/copy.rs:109:33
    |
109 | unsafe impl ArchiveCopySafe for NonZeroU8 {}
    |                                 ^^^^^^^^^ the trait `ArchiveCopy` is not implemented for `NonZero<u8>`

@benwis
Copy link
Contributor

benwis commented Feb 4, 2024

Clippy is complaining about an issue inside rkyv, here and everywhere.

I am just starting off the bug hunt investigation .. and reporting what I see as I go Sometimes just talking about it in a reasoned way allow others to effortlessly jump directly to the right solution.

In the build artefacts I see this error

running `cargo clippy --no-default-features --all-features` on leptos (19/120)
error: process didn't exit successfully: `/home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo clippy --manifest-path /home/runner/work/leptos/leptos/leptos/Cargo.toml --no-default-features --all-features` (exit status: 101)

From the main branch, here is what I interpret as the failing command

cargo +nightly clippy --no-default-features --all-features

so we can peek what is behind the (exit status 101).

I get a long list of errors, all of the the same form. So I am just recording the first two errors below

They are rkyv-0.7.43 related

When I look at the full list of errors, looking for a entry point, a place in the leptos code which is a root of all this ... I get no joy...

What do we need to do to get rkyv to compile? in this instance

So I am expecting a the answer to be and complex nested inter-dependency where a sub module we import has a bug and is not setting the correct feature flags to ensure compilation.

When I use cargo why rkyv or cargo tree to grapple with these issues. I am doing something wrong as I can I see no dependence on rkyv.... so any advice would be welcome

error[E0277]: the trait bound `NonZero<i8>: ArchiveCopy` is not satisfied
   --> /home/martin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rkyv-0.7.43/src/copy.rs:108:33
    |
108 | unsafe impl ArchiveCopySafe for NonZeroI8 {}
    |                                 ^^^^^^^^^ the trait `ArchiveCopy` is not implemented for `NonZero<i8>`
    |
note: required by a bound in `ArchiveCopySafe`
   --> /home/martin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rkyv-0.7.43/src/copy.rs:99:35
    |
99  | pub unsafe trait ArchiveCopySafe: ArchiveCopy + Sized {}
    |                                   ^^^^^^^^^^^ required by this bound in `ArchiveCopySafe`

error[E0277]: the trait bound `NonZero<u8>: ArchiveCopy` is not satisfied
   --> /home/martin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rkyv-0.7.43/src/copy.rs:109:33
    |
109 | unsafe impl ArchiveCopySafe for NonZeroU8 {}
    |                                 ^^^^^^^^^ the trait `ArchiveCopy` is not implemented for `NonZero<u8>`

I wouldn't worry about this, making clippy happy with rkyv is not required for us to merge it

@gbj
Copy link
Collaborator

gbj commented Feb 4, 2024

Thanks, this looks good to me!

@martinfrances107 rkyv is a dependency of server_fn. We should probably change the CI so that it uses the equivalent of cargo clippy --no-deps and doesn't lint our dependencies in any case, but I don't see an obvious way to do that with cargo-hack, which is what's used at present.

I'll just run CI locally and merge.

@gbj gbj merged commit 85c3755 into leptos-rs:main Feb 5, 2024
44 of 60 checks passed
videobitva pushed a commit to videobitva/leptos that referenced this pull request Feb 17, 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.

None yet

4 participants