Navigation Menu

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

'*' wildcard #60

Closed
cevr opened this issue Feb 27, 2019 · 12 comments
Closed

'*' wildcard #60

cevr opened this issue Feb 27, 2019 · 12 comments

Comments

@cevr
Copy link
Contributor

cevr commented Feb 27, 2019

I can't seem to find this in the docs, but is it possible to route with a '*' wildcard that acts as a middleware?

Example:

const routes = mount({
    '/login': route({
        view: <div>Login</div>,
    }),
    '/': redirect('/login'),
    '*': map((req, { user }) => (user ? protectedRoutes : redirect(req.originalUrl))),
});
@jamesknelson
Copy link
Collaborator

It's not possible right now, but it'd be a simple enough addition. The only thing that would need to be changed is to add a special case if (pattern === '*') to createMapping() here:

https://github.com/frontarm/navi/blob/master/packages/navi/src/Mapping.ts#L59

Would you be interested in submitting this is a PR?

@cevr
Copy link
Contributor Author

cevr commented Feb 28, 2019

It's not possible right now, but it'd be a simple enough addition. The only thing that would need to be changed is to add a special case if (pattern === '*') to createMapping() here:

https://github.com/frontarm/navi/blob/master/packages/navi/src/Mapping.ts#L59

Would you be interested in submitting this is a PR?

Sure I'd love to. I have some questions though, would the '*' wildcard just short circuit the createMapping function, immediately returning the mapping? I'm not entirely sure what is going in the createMapping function.

@jamesknelson
Copy link
Collaborator

So createMapping() is taking a pattern, and computes an object that holds information about that pattern. That object is then used by the mount() function.

The special thing about '*' is that it would gobble up '/' characters, which no other patterns can do, so it would probably be easiest to just special case it at the top of the function.

@nshoes
Copy link

nshoes commented Mar 1, 2019

@cevr Just curious, why the need for a * matcher? Why not wrap your routes in a reusable map?

Because of it's compostability you can even compose or pipe multiple map's together to suit your needs.

@jamesknelson
Copy link
Collaborator

Say you have a few routes that you want to be available without auth (e.g. / and /login in the above example), but you want the rest to require auth. In that case, you might want to split the authenticated and non-authenticated routes into two mount() calls, and wrap the map() around just one of them.

I think you raise an interesting point though -- this does feel more natural to me:

compose(
  mount({
    '/login': route({
      view: <div>Login</div>,
    }),
    '/': redirect('/login'),
  }),

  // Fall back to this if no route was found in the `mount()`
  map((req, { user }) => (user ? protectedRoutes : redirect(req.originalUrl))),
)

Looking at this though, I've realized an issue with both this approach and the * approach: it's going to be tricky to build a site map.

Currently, it's possible to recursively look through all the patterns of a mount() and use that to build a list of all possible URLs (i.e. a site map). This is how the static renderer knows which URLs to render. Adding support for * or mount/map fallbacks is going to break this, and I'm not exactly sure how to fix it...

@nshoes
Copy link

nshoes commented Mar 1, 2019

it's going to be tricky to build a site map

Nearly impossible right? Haha. What if some convention and helpers were exposed so 1.) this pattern would be easier/more obvious to implement and 2.) we could infer that they are not requiring auth for a few routes and are for the rest. I'm not sure if that's even possible 🤹‍♂️, apologies if it's way off base.

@cevr
Copy link
Contributor Author

cevr commented Mar 1, 2019

@jamesknelson I see what you mean. I think I ran into that problem while trying to do a workaround method (site map not detecting other routes). Perhaps the better solution would be to mount different routes at the root index.js, though I haven't tried it. Thoughts?

const bootstrap = user => {
    const routes = user ? protectedRoutes : unprotectedRoutes;
    register({
        routes,
        exports: {
            App,
        },
        async main() {
            let navigation = Navi.createBrowserNavigation({ routes });
            navigation.setContext({ user });

            await navigation.steady();

            let hasStaticContent = process.env.NODE_ENV === 'production';
            let render = hasStaticContent ? ReactDOM.hydrate : ReactDOM.render;

            render(
                <NaviProvider navigation={navigation}>
                    <App />
                </NaviProvider>,
                document.getElementById('root'),
            );
        },
    });
};

getUser().then(bootstrap)

@nshoes
Copy link

nshoes commented Mar 1, 2019

mount different routes at the root index.js

What if the user signs in or out? How would you re-render the whole app? Reload the page? Seems like this could be solved without re-mounting everything.

@jamesknelson
Copy link
Collaborator

The way I currently do this is to just protect routes one at a time; so instead of wrapping a mount() with a map() that redirects when unauthenticated, I wrap individual routes. I go into the details in this guide.

You could even create a custom withAuthentication() matcher that can be composed together, if that simplifies things.

I'm currently putting together an example using firebase for auth, and hope to have it done this weekend. That would probably be the best way for me to demonstrate.

it's going to be tricky to build a site map

Nearly impossible right? Haha.

I've been thinking about this and I think it should still be possible by using custom headers or a custom HTTP method. E.g. use get to see if there is a page at the URL, and use map to find a list of URLs at the URL. In the meantime it'd be easier to just avoid the issue, though.

@jamesknelson
Copy link
Collaborator

This is currently blocked by #64. Once the new site map generation is done, wildcards or mount composition should become an easy change.

@jamesknelson
Copy link
Collaborator

Just an update on this: we're going to have to go with wildcards instead of mount composition, as mount composition is incompatible with react-router style <Route> components -- and I'd like to add them at some point.

jamesknelson added a commit that referenced this issue Mar 17, 2019
@jamesknelson
Copy link
Collaborator

Have just pushed wildcard support, will be part of the next release.

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

No branches or pull requests

3 participants