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 unnecessary wrapper #438

Closed
wants to merge 1 commit into from

Conversation

jerrygreen
Copy link

Honestly initialize function doesn't seem to do anything, while templating code only creates additional wrapper (div), - which only makes the situation with html even worse. While JS has callback hell problem, HTML has div wrapper hell problem. Thus, the code removes unnecessary initialization code (prove me wrong), and removes the wrapper.

@jakobrosenberg
Copy link
Member

Thanks for the PR. I'm curious, how does the app navigate without

router.url.push(url)

Does it reload the whole app on each page change?

The DIV wrapper is currently required to avoid the clicks inside one router to bubble up to the parent routers. For most projects that's not an issue, but for apps using nested router instances, encapsulation is required. One example of nested routing is the the docs site for R3, where each example is a nested router. https://v3.ci.routify.dev/docs#guide/introduction/structure

If there's an issue with the current wrapper, I'll be happy to help with a solution.

Function `initialize` now listens to the document event, not element wrapper.
One less div wrapper, hooray!
@jerrygreen
Copy link
Author

jerrygreen commented Mar 26, 2022

You're right, I'm wrong: this code, indeed, needed for nested routes. By some reason it worked just fine for non-nested routes, so I didn't notice the change.

Off-topic: that's a question why... Why it does not make full page reload for non-nested routes?
But I don't urge you to answer that since it's not related to the point where the wrapper is unnecessary.

I made a change that it now listens to the document, not some very parent body-like tag wrapper. Tried it on a few links, nested, non-nested: seems to work fine. Any other remarks?

@jakobrosenberg
Copy link
Member

The problem with using document instead of a scoped element is that all routers then listen to the same events. That means whenever you click a link, all routers will navigate to it.

Clicks need to be scoped/restricted to their router in some way. There are some other alternatives like detecting the parent element of a router, but this would prevent multiple routers from being nested inside the same element. That shouldn't be an issue though as it's an edge case and could be handled by creating a div like wrapper for each sibling router.

The big question then is, how does a Svelte component resolve its parent element? @ghostdevv?

@jerrygreen
Copy link
Author

jerrygreen commented Mar 26, 2022

Honestly I’m little confused when you say «router» and especially «routers», because previously you meant «route» and «routes».

route(s) - a folder structure that translates into an accessible url(s)
nested route(s) - a route structure that is more than one level deep in folders
router - a component that user imports from routify library, and applies to his code, which brings him the NextJSy routes that we all like (that I’m trying to change here, so it no more requires a wrapper div)

And when you say «routers», - I don’t understand if you mistakenly call routes routers again, or if it’s another thing now. It seems you mean the router component itself now, rather than routes… But honestly I don’t know if it’s even possible to use more than one routER, and if possible, - why would anyone do that? Maybe I just don’t know about some ricky-tricky-edge-case, but honestly it sounds like nonsense. I might be wrong though, but please then explain.

@ghostdevv
Copy link
Member

ghostdevv commented Mar 26, 2022

I don't see how you would be able to scope the clicks per router without knowing what router they are relevant to. At a guess since Svelte doesn't have a single root element per component I would imagine it goes based of the target property on the component constructor

EDIT: I would say that an alternative would be good, especially since currently the handlers aren't removed when the div is unmounted.

@ghostdevv
Copy link
Member

@jerrygreen in Routify 3 it is possible to run multiple Routify instances (routers) in the same page. This is a side effect of our move to make Routify more customisable and flexible, the target audience are more advanced applications that might have multiple teams working on seperate parts of the application or something like widgets.

@jerrygreen
Copy link
Author

jerrygreen commented Mar 29, 2022

I would say that an alternative would be good, especially since currently the handlers aren't removed when the div is unmounted.

So does that mean we should merge the change?


Btw, I just tested how it works with a few router components being used at the same time. They store their paths in location separately using ; separator, like this:

http://localhost:3000/menu;/menu

Honestly this current behavior is just weird, non-web, and simply above my understanding, and honestly I don't even want to understand that: it is some very edgy case (/menu;/menu urls? definitely an edgy case).

With this PR, it works still little similar but not entirely similar: it slightly breaks the current behavior.

If you want it to be non-breaking, please feel free to adapt it for your needs, for your picture of how this supposed to work, or close the PR: that's up to you. My only call: the wrapper div is not necessary, and shouldn't be applied (pls don't add to the div-hell problem). And thx for the lib anyway.

@jakobrosenberg
Copy link
Member

Router: an instance of Routify that renders a route
Nested router: a router living inside another router (like the examples here, each example is a new instance of Routify. If we used document instead of the parent div, each example plus the entire page would react to every navigation click.)
Route: an object containing an array of nodes (components) corresponding to the router's current URL. This array gets rendered by the router.

Routify 3 was always intended to be a highly flexible routing powerhouse. Apps, dashboards, "common" edge cases etc. are at least as important to us as plain websites.

I'm still open to a solution to the div, as long as it's not at the expense of core functionality. One solution may be an option like eventsScope: 'scoped'|'global'.

@ghostdevv
Copy link
Member

I don't see how we can remove the div without making extra work, unless we move the div to another place in the component.

<div style="display: none;" use:initialize />

@jakobrosenberg
Copy link
Member

jakobrosenberg commented Mar 30, 2022

Is there anyway to detect the parent element of a Svelte component?

Edit: If not, we can do the R2 trick of spawning an element, fetching its parent and then deleting the element.

@ghostdevv
Copy link
Member

@jakobrosenberg not without a element to run the js on xD

@ghostdevv ghostdevv closed this May 24, 2022
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