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

Added Support for query parameters #19

Closed
wants to merge 8 commits into from
Closed

Added Support for query parameters #19

wants to merge 8 commits into from

Conversation

koa
Copy link
Contributor

@koa koa commented Oct 23, 2024

I found no solution without an API change.
The Implementation is still rough, but the unit tests run through.
Could be a solution for #9

Possible I killed a feature by ignoring converter and target_converter at https://github.com/koa/yew-nested-router/blob/719392b53860046425eca288a4416af8f7aa8b54/yew-nested-router-macros/src/lib.rs#L337, so i leaved the parameters for the moment.

@ctron
Copy link
Owner

ctron commented Nov 5, 2024

Sorry, I dropped the ball on this.

I like the PR, and an API change is just fine. We can make a new version and that's it.

I am worried of breaking something:

Possible I killed a feature […]

😬

What do you think is broken?

@koa
Copy link
Contributor Author

koa commented Nov 12, 2024

In my current (work-)project I don't need the Link-Component (because I implement only webcomponents in yew, integrated into a bigger angular application, so the router is implemented in angular).
I could find no use for the parameters "converter" and "target_converter" on the function "parse_rules" of the macro. So I suspect I am missing something.
Additionally I fixed just yesterday the support for collections of parameters.
In my private project, for sure I will need the Link-Component and the Router too, but I had no time to make progress there. So there is no experience either.

src/components/link.rs Outdated Show resolved Hide resolved
src/components/link.rs Outdated Show resolved Hide resolved
let router = match use_router::<T>() {
None => {
error!("No router");
return html! {"Need Router or Nested component"};
Copy link
Owner

Choose a reason for hiding this comment

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

I kind of like the idea of not-panicking.

@ctron
Copy link
Owner

ctron commented Dec 19, 2024

What's the state of the PR. I am ok merging, even with breaking changes. As long as the other stuff still works.

@koa
Copy link
Contributor Author

koa commented Dec 19, 2024

It is a mixed bag. Partially it is already rolled out in a productive project, but I have newer used in a standalone yew application, so there could be some horrible undetected bugs.
If you are agree with that risk, I will have no problems too.

@ctron
Copy link
Owner

ctron commented Dec 19, 2024

It is a mixed bag. Partially it is already rolled out in a productive project, but I have newer used in a standalone yew application, so there could be some horrible undetected bugs. If you are agree with that risk, I will have no problems too.

That's fair. I'll make it a breaking release then, so that people need to opt-in.

@ctron
Copy link
Owner

ctron commented Dec 19, 2024

I think there are two warnings in the generated code:

warning: unused variable: `converter`
   --> /home/runner/work/yew-nested-router/yew-nested-router/yew-nested-router-macros/src/lib.rs:349:5
    |
349 |     converter: F1,
    |     ^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_converter`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `target_converter`
   --> /home/runner/work/yew-nested-router/yew-nested-router/yew-nested-router-macros/src/lib.rs:350:5
    |
350 |     target_converter: F2,
    |     ^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_target_converter`

warning: `yew-nested-router-macros` (lib) generated 2 warnings

@koa
Copy link
Contributor Author

koa commented Dec 19, 2024

Yes. I couldn't fined out, why these parameters where needed, so when someone finds it out, he can easier fix the problem.
If they are really not needed, I will remove them immediately (See my comment in the original post).

@koa
Copy link
Contributor Author

koa commented Jan 8, 2025

The PR is not ready now. I will send it again, when it is more ready.

@koa koa closed this Jan 8, 2025
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.

2 participants