remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.03k stars 10.31k forks source link

Question about <Miss /> #3914

Closed acdlite closed 8 years ago

acdlite commented 8 years ago

<Miss /> breaks slightly with my conceptual model of React components, which is that they are functions of data that return UI. I believe this intuition of mine is borne out by the fact that it's not currently possible to render <Miss /> in a single pass. This isn't great in general, but it's a problem for server-side rendering especially.

While I remain open-minded about the ergonomic value of a <Miss /> component, and it's possible that a future version of React will have APIs that are more accommodating to implicit cross-children communication, I'd like to pose a question.

Is there anything that <Miss /> solves that isn't solved using an API like the following?

// Before: with <Miss /> (taken from v4 docs)
<Match pattern="/" exactly component={Home}/>
<Match pattern="/will-match" component={WillMatch}/>
<Miss component={NoMatch} />

// After: with single <Match /> and routes
<Match
  routes={[
    { pattern: '/', exactly: true, component: Home },
    { pattern: '/will-match',  component: WillMatch }
  ]}
  miss={{ component: NoMatch }}
/>

(This looks similar to the <MatchWithRoutes /> component here except without subroutes and with the addition of a miss prop. Not sure how this proposal would or should affect that project.)

The big advantage is that the <Match /> in this example knows whether it should render the miss before anything else is rendered, enabling single-pass render without lifecycle methods or subscriptions.

The only disadvantage I can think of is that it's uglier—arguably. It looks fine to me but I know how much people love their JSX.

I haven't thought much about this yet, so it's likely I'm neglecting other reasons why this won't work.

Please note that this issue is meant less to propose a specific API and more to provoke discussion over the conceptual model of <Match />s and <Miss />s.

timdorr commented 8 years ago

<Match> is intended to be used anywhere in the component tree. If you want to have two <Match>s in different areas of the tree along the same path context (see <MatchProvider> in the source), then this kind of communication isn't possible other than going along this.context (which it does). That is the point of having a separate component, rather than putting it as a prop: You don't have to centralize your route config anymore and can compose routing however it best suits your application. And that might be in multiple ways, where you have a top level, page-oriented route config and then <Match>s in various places to change up a component sub-tree based on the URL (think of a settings page with multiple tabs).

We moved away from that central config because it's restricting and actually lies a bit about what's going on underneath. We're not really reading those <Route> components as React components. It's actually just that we support both React elements and POJOs as a route config. <Route> doesn't even render! We're now embracing React's awesome declarative, composable nature.

Context is currently the best way to get from point A to point Q in the component tree. It suits our needs pretty well to isolate sub-trees as being under a particular pattern and have a set of matched patterns in that sub-tree, no matter where they show up. I think that kind of composition is important to maintain to both keep things clear and keep working with the router a fun experience.

Server rendering might be a little harder right now, but I think some of the projects coming along (like react-router-addon-routes) will make that easier to work with. Previous versions didn't even have real 404 handling (not without some serious trickery), so I think we're still ahead of the game here. But let's wait for people to figure out the 2-pass rendering hack before we throw <Miss> under the bus. I feel like there's another step or two to go until those things are made clear.

acdlite commented 8 years ago

@timdorr C'mon Tim. I hate to be that guy, but did you actually read what I wrote? 💃

I don't think you have addressed anything that I actually said. I'm not proposing moving to a static or central config. My <Match routes /> example can still be nested, composed, whatever.

I'm also not proposing moving away from context for <Match />. Indeed, I didn't mention context at all! My issue is entirely about how <Miss /> works.

I mean this in the most polite way possible, but would you mind reading my comment again and responding to the actual issue? :) I'd love to know your perspective.

timdorr commented 8 years ago

😿 Sorry, I thought I did. I'll try again, though. Not intending bury the lede or misdirect, I swear!

The bigger problem I think there is with putting miss as a prop is that you now tie the miss display to one <Match> at a time. If you want to put <Match> in multiple places within the same "match context" (e.g., under a <MatchProvider>), you wouldn't be able to stop it from showing every one of those miss components. It's a matter of coordination.

As an example, imagine if you had two side-by-side tabbed panels to that settings page I had mentioned before. If you want to have a roll-up 404 display under that page that replaces both panels, you wouldn't be able to do that with the miss prop. Sorry if that example isn't clear. We can try something else if it works better.

I think the worse sin would be to depend on ordering for how <Miss> rendering in order to get a one-pass workflow. For example:

<Match pattern="/" exactly component={Home}/>
<Match pattern="/foo" component={Foo}/>
<Miss component={NoMatch} /> // Renders because no match up until this point!
<Match pattern="/bar" component={Bar}/>

Yeah, that would be scary, wouldn't it? 😱 And I know all the fiber stuff coming could even make this an even more quirky behavior (since rendering order doesn't have to be deterministic anymore).

So, I think it really has to stay as a two-pass render and something that lives outside of <Match> to be truly useful. Otherwise it feels pretty close to what was being requested in #3906. And that can be built in userland, so we don't want that in the core just yet.

Obviously, it's still ugly for the server side and the conceptual level, but I think we'll get there over time as everyone wraps their head around this new router.

timdorr commented 8 years ago

And a question back to you, if you don't mind: Doesn't the fiber architecture lend itself towards the kind of asynchronous rendering that <Miss> relies on? If so, do you think that fits in better with where the internals are headed? I don't think we need to fully embrace async rendering (that's what <4.0 did and it's kind of a mess!), but it seems like it potentially fits within the unit of work concept as something that can be rendered after other parts can be rendered.

Maybe if that kind of API was exposed to components, we could say "render <Miss> last" so that we don't have to do a two-pass render?

Am I taking crazy pills? :)

acdlite commented 8 years ago

Sorry, I thought I did. I'll try again, though. Not intending bury the lede or misdirect, I swear!

You probably addressed it more than I gave you credit for. I just felt slightly patronized with all the business about how awesome declarative composability is, which I don't think my proposal contradicted at all. No worries, pal :)

As an example, imagine if you had two side-by-side tabbed panels to that settings page I had mentioned before. If you want to have a roll-up 404 display under that page that replaces both panels, you wouldn't be able to do that with the miss prop.

Hmm, I think I see what you mean now. It would be cool to have an example in the docs that shows this use case. AFAICT all the existing examples would work with a miss prop.

I think the worse sin would be to depend on ordering for how rendering in order to get a one-pass workflow.

<Miss> has this problem, but a miss prop does not, right?

So this leads me to a few other questions to ponder

Again, I'm not sold on a miss prop. I just want to explore other options for that API, because I'm not convinced <Miss> is ideal.

acdlite commented 8 years ago

Doesn't the fiber architecture lend itself towards the kind of asynchronous rendering that relies on?

Yes but single-pass rendering will still be important for a streaming server-side renderer.

steida commented 8 years ago

Double rendering is the only way how to check render "state" after render. I used it a lot in Este for data fetching without React Router. React itself will support it natively soon. I suppose what @acdlite wants is more strict routing. It's the relevant use case and it would also (temp) fix https://github.com/ReactTraining/react-router/issues/3905 and https://github.com/ReactTraining/react-router/issues/3877. MultiMatch with default case would be nice and also React idiomatic.

acdlite commented 8 years ago

React itself will support it natively soon

What are you referring to? Double rendering on the server is definitely not recommended.

steida commented 8 years ago

@acdlite https://twitter.com/sebmarkbage/status/773557813033340928?lang=en and https://github.com/facebook/react/issues/7671

acdlite commented 8 years ago

@steida As I understand it, those new hooks not intended for you to call setState or to in any other way re-render. They're more for you to collect data and use it to build up some other part of the HTML payload (stuff in the <head> for instance). I doubt double rendering on the server will ever be explicitly supported.

steida commented 8 years ago

Maybe it will not, but who cares? It's pretty fast because the first render has no data to be rendered, so it takes several ms. But I see another problem with the Miss.

<Miss render={() => <Redirect to="/" />} />

Imagine we have such code at several places in the app, of course with different to prop. The order of redirections can't be ensured nor controlled.

timdorr commented 8 years ago

@steida I care! 😄 I don't want my server doing two passes of rendering, because both will require roughly the same amount of work. renderToString doesn't retain any state from execution to execution. We hack around this now with the external "server context", but it's definitely not pretty.

And it can potentially cause problems with duplicate requests being made to backends or other non-idempotent resources being used during each render.

steida commented 8 years ago

@timdorr I think you read it to fastly. I am using double rendering and was discussing it with Ryan several months ago. No, it can't potentially cause problems with duplicate requests. Context is scoped per request.

ryanflorence commented 8 years ago

I have a ton of thoughts on this, and have even done a PoC of what Andrew has shown, and had a bunch of conversations with Sebastian about the Miss API. I'm on vacation right now though so you'll have to wait! On Mon, Sep 19, 2016 at 2:19 PM Daniel Steigerwald notifications@github.com wrote:

@timdorr https://github.com/timdorr I think you read it to fastly. I am using double rendering and was discussing it with Ryan several months ago. No, it can't potentially cause problems with duplicate requests. Context is scoped per request.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ReactTraining/react-router/issues/3914#issuecomment-248130195, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGHaDQf8CTlELeJqAk6XpeY_RoEkwT9ks5qrvxRgaJpZM4KA1rr .

ryanflorence commented 8 years ago

I just don't want anybody thinking anybody has come to any conclusions yet, we haven't. On Mon, Sep 19, 2016 at 2:23 PM Ryan Florence rpflorence@gmail.com wrote:

I have a ton of thoughts on this, and have even done a PoC of what Andrew has shown, and had a bunch of conversations with Sebastian about the Miss API. I'm on vacation right now though so you'll have to wait! On Mon, Sep 19, 2016 at 2:19 PM Daniel Steigerwald < notifications@github.com> wrote:

@timdorr https://github.com/timdorr I think you read it to fastly. I am using double rendering and was discussing it with Ryan several months ago. No, it can't potentially cause problems with duplicate requests. Context is scoped per request.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ReactTraining/react-router/issues/3914#issuecomment-248130195, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGHaDQf8CTlELeJqAk6XpeY_RoEkwT9ks5qrvxRgaJpZM4KA1rr .

timdorr commented 8 years ago
  • How common is that use case? Is it worth sacrificing one-pass rendering?
  • Is there another way we can address that specific use case that has fewer caveats?

So, my estimation is that most folks want to return a true 404 page, with a real HTTP status code and everything. Just like they did in the old days of 2014! So, I think the double-render is actually a misdirection of sorts. I really hope most will do this:

if (result.missed)
  return res.sendStatus(404).send(renderToString(<NotFound/>)

That is, they would be rendering something else if there was a 404.

And this feeds back into the top-level routes concept. I think once something is in place for that, using <Miss> might end up being sporadic enough that it can be dropped in favor of some top-level route resolver's own version of a "miss".

But yeah, let's see what Ryan's got cooking. He and @mjackson have definitely been thinking about this longer than either of us (I think they started on this back in April?), so there is probably another level above this that we're not even thinking about yet :)

timdorr commented 8 years ago

@timdorr I think you read it to fastly. I am using double rendering and was discussing it with Ryan several months ago. No, it can't potentially cause problems with duplicate requests. Context is scoped per request.

If a resource is accessed by a component along the render code path, and if that resource commits some non-idempotent action when accessed, then it certainly could cause problems. It's basically causing your initial render itself to not be idempotent by proxy. That can certainly happen.

ryanflorence commented 8 years ago

Alright ...

So, yeah, Match/Miss is a really new idea, or is it? Check out this plain ol' html form:

<form>
  <input type="text"/>
  <input type="text"/>
</form>

If you keydown Enter while one of those inputs is focused, nothing happens. But when you add a button:

<form>
  <input type="text"/>
  <input type="text"/>
  <button type="submit">Save</button>
</form>

KABOOM! Suddenly a keydown of Enter causes the parent form to submit! The presence of another descendant of the form changes the behavior of other descendants.

It's the same idea with Match and Miss. A relationship between descendants in the same context for a Router instead of a form.

Originally, I was able to get it to work flawlessly in a single render pass. It used componentWillMount to register the number of successful matches in context, and if there were none, then Miss could check that count from context and knew that it should render because render was called after componentWillMount of all other descendants.

It was beautiful 😭 (:tears-of-joy:)

Then I went and had a conversation with @sebmarkbage about fibers and he told me that componentWillMount might actual be lying to us, and the component may never mount even though it told us it was going to 😂 (:tears-of-confusion:).

So, first, I made a rap about it "Component Gonna Mount (Suckahs)": https://twitter.com/ryanflorence/status/774481874735370240

After that, I changed the code here 227a2edf2ab9e24c9174056421db68f7faf74c55

While talking to Sebastian he said he'd liked the Match/Miss API, so I'm hoping we can figure out a better way to do it after the dust settles, the only real rub is with server rendering, in the client everything seems exactly the same as before.

I just merged in MatchGroup, that, with a bit more work, could handle misses in a single pass as well.

<MatchGroup>
  <Match exactly pattern="/" component={Home}/>
  <Match pattern="/foo" component={Foo}/>
  <Miss component={NoMatch}/>
</MatchGroup>

Right now it's really simple, finds a match, then just renders that child, or if there's no match, renders the miss, so all lifecycles and stuff are still happening. With a bit more finesse we can get it to do the rendering itself the same way Match and Miss do, and we'd basically just be stripping props from them to make it happen, but that would mean breaking composability just like our Route components did. But that way we'd be able to get some Miss happening in one shot.

I want to let this bake a bit, build some server rendering examples, and wait to see what is coming out of fibers before worrying much more about this. Rendering a 404 status code is an exceptional case already, and that's really all we're talking about here. Maybe something in fibers will save us, maybe we'll need to adjust MatchGroup to fully support exceptional server rendering.

We don't have all the answers yet, but they will come. Thanks for asking the question Andrew!

I'm gonna close this ... now.

Also, I've had "Component Gonna Mount" on repeat as I typed the whole thing out 😂

sebmarkbage commented 8 years ago

There is a pattern with ComponentKit, actually used in our News Feed, that does something like this:

return <Foo props={data} /> || <Bar props={data} /> || <Baz props={data} />;

In that environment, components gets executed inline so you can see if it ultimately renders to null and if so, choose a different branch. It allows you to optimistically render components as a best-effort but if they fail to render something useful, you can render something else.

In imperative immediate mode GUIs you sometimes see people do:

let rendered : boolean = tryToRenderButton(data);
if (!rendered) renderEmptySpace();

In a functional GUI where it is just functions calling functions, you could do something like this:

let maybeOutput = tryToRenderButton(data);
if (!maybeOutput) return renderEmptySpace();
return maybeOutput;

Of course, you might want it to be more explicit states than null but the principle is the same. On the way up, you can reason about the return value. In fact, this is exactly what coroutines in Fiber lets you do.

So there's something intriguing about this pattern. There's something unsettling about the logic for this being highly disconnected in the tree without anything to tie them together in the code visually. I'd probably want the <Miss /> as the parent context to all the matchers just to avoid it spreading all over the place. E.g:

return (
  <div>
    <Miss render={<ContentThatMightContainMatchers />} else={<AlternativeContent />} />
  </div>
);

Or something like that.

But that's a minor point in comparison.

ryanflorence commented 8 years ago
return (
  <Match /> ||
  <Match/> ||
  <Match/> ||
  <Match/> ||
  <NoMatch/>
)
acdlite commented 8 years ago

Ooh, I like the coroutine idea

jedwards1211 commented 7 years ago

@ryanflorence doesn't <Match /> || <Match /> || <NoMatch /> suffer from the same exact limitations as @acdlite's original proposal? <NoMatch /> is dependent on all those <Match />es being rendered in the same place as it.

jedwards1211 commented 7 years ago

@ryanflorence while we're only talking about 404s here, I can imagine other use cases for <Miss>-like behavior. The primary example that comes to mind is rendering member pages of a site inside one container, and public pages without that container. Of course one could just put all member pages under the /members route...but if one wants member pages to be top-level, then it's pretty hairy.

jedwards1211 commented 7 years ago

I had a thought about the double-rendering concerns: one could set a context variable on the first pass that tells components it's the first pass -- and they could respond by rendering only the routes and data subscription components necessary for the first pass to get info needed for the second pass, without any of the content that's just for humans :)

I'm glad to see that double-pass rendering is becoming more acceptable because I think it makes isomorphism rendering/data fetching a lot easier.