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

Factor out support for query_arg binding from rest of router changes #1892

Closed
wants to merge 4 commits into from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented May 31, 2021

Description

This is #1860 minus the following features:

  • "Mount" support.
  • State support.

It is based on #1891

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@Diggsey Diggsey force-pushed the router-rewrite-partial branch 2 times, most recently from 7addcf5 to bee2700 Compare May 31, 2021 22:56
Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

I left some comments. This looks good for the most part. I feel like some items deserve their own modules (like Route) and there were things that were exposed which shouldn't have been. Also, please update the macro tests.

packages/yew-router-macro/src/routable_derive.rs Outdated Show resolved Hide resolved
packages/yew-router-macro/src/routable_derive.rs Outdated Show resolved Hide resolved
packages/yew-router-macro/src/routable_derive.rs Outdated Show resolved Hide resolved
packages/yew-router/src/hidden.rs Outdated Show resolved Hide resolved
packages/yew-router/src/hidden.rs Outdated Show resolved Hide resolved
packages/yew-router/src/history.rs Outdated Show resolved Hide resolved
packages/yew-router/src/lib.rs Outdated Show resolved Hide resolved
packages/yew-router/src/service.rs Outdated Show resolved Hide resolved
packages/yew-router/src/utils.rs Outdated Show resolved Hide resolved
packages/yew-router/src/hidden.rs Outdated Show resolved Hide resolved
@Diggsey Diggsey force-pushed the router-rewrite-partial branch 6 times, most recently from 3247acc to 7c548d4 Compare June 9, 2021 22:31
@Diggsey
Copy link
Contributor Author

Diggsey commented Jun 9, 2021

Tests are now passing. The only remaining issue is the naming of "munchers".

@ranile
Copy link
Member

ranile commented Jun 10, 2021

Great, can you update the docs too? Those will need a good amount of updates.

Also, I will try to do the second round of review as soon as I get some time (pretty busy with other stuff right now)

@Diggsey
Copy link
Contributor Author

Diggsey commented Jun 10, 2021

👍

@Diggsey Diggsey force-pushed the router-rewrite-partial branch 3 times, most recently from de24938 to b0fee4b Compare June 12, 2021 22:44
@github-actions
Copy link

github-actions bot commented Jun 12, 2021

Visit the preview URL for this PR (updated for commit 4d2e27c):

https://yew-rs--pr1892-router-rewrite-parti-4iny57da.web.app

(expires Wed, 23 Jun 2021 22:42:50 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@Diggsey
Copy link
Contributor Author

Diggsey commented Jun 12, 2021

I've performed the renaming and updated the documentation. That should address everything you've brought up so far.

Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

I feel like HistoryState and RouterState are too abstracted and complicated? Can we simplify those any more? Want me to give it a shot?

This review is incomplete. This is what jumped out to me when looking though the code.

packages/yew-router/src/service.rs Show resolved Hide resolved
packages/yew-router/src/service.rs Outdated Show resolved Hide resolved
packages/yew-router/src/history.rs Show resolved Hide resolved
packages/yew-router/src/service.rs Outdated Show resolved Hide resolved
@Diggsey
Copy link
Contributor Author

Diggsey commented Jun 13, 2021

I feel like HistoryState and RouterState are too abstracted and complicated? Can we simplify those any more? Want me to give it a shot?

They both implement the same rules, which are as follows:

  • A given "state" object is either active or inactive.
  • When a "state" object is active, it stores the current value, and is subscribed to changes to that value.
  • When a "state" object is inactive, it stores no value, imposes no overhead and is not subscribed to any events.
  • When an active "state" object is notified of a change, and there are no downstream subscribers, it becomes inactive.
  • When any API call is made which relies on the "state" object, and it's currently inactive, it becomes active.

These rules ensure the router does the minimum amount of work for any sequence of calls to its API (since the current value will always be cached as long as it is still valid) whilst minimizing the overhead of the router when it is not in use, and ensuring that it cleans up after itself gracefully.

The difference between HistoryState and RouterState is that HistoryState is a singleton, whereas RouterState is parameterized by the user-defined Routable type, but otherwise they implement the same state machine.

The code maps 1:1 onto that state machine: the constructor is used to "activate" the state. The Drop imeplementation is used to "deactivate" the state. The "update" method is called when an event is received.

If you can simplify the code whilst implementing the same state machine, and without breaking the 1:1 mapping between the code and the state machine it implements, then that would be worthwhile.

@ranile
Copy link
Member

ranile commented Jun 13, 2021

I was thinking of simplifying it in a way that the router's state is stored inside the router, not globally. Is that something that can happen? Locally managed state is better than global state.

@Diggsey
Copy link
Contributor Author

Diggsey commented Jun 13, 2021

I was thinking of simplifying it in a way that the router's state is stored inside the router, not globally.

The router API is global, so even if you were able to do that, you'd just be lying to users about where the state lives...

Regardless, I think this should be global state, since it is state which is derived from window.location, which is itself a global property.

@mc1098 mc1098 added A-yew-router Area: The yew-router crate A-yew-router-macro Area: The yew-router-macro crate labels Sep 20, 2021
@voidpumpkin
Copy link
Member

I believe correct way of implementing this would be to to first contribute to route-recognizer and add proper query params parsing http-rs/route-recognizer#47

Also I'm closing this PR as it is stale for a long time
@Diggsey ping me if you make updates and want this reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-router Area: The yew-router crate A-yew-router-macro Area: The yew-router-macro crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants