nytimes / redux-taxi

🚕 Component-driven asynchronous SSR in isomorphic Redux apps
Other
71 stars 8 forks source link

Is this used in production at NYT? #1

Closed jaredpalmer closed 7 years ago

jaredpalmer commented 8 years ago

I want to benchmark it vs. my own ssr react boilerplate.

evanhobbs commented 8 years ago

Curious about this as well...

tizmagik commented 8 years ago

Sorry for the late reply. Yes, it's currently used in some sections of the myaccount portion at The Times.

tizmagik commented 8 years ago

@jaredpalmer would love to hear the results of your benchmark.

jaredpalmer commented 8 years ago

@tizmagik are you guys also lazy loading your routes and injecting reducers?

jaredpalmer commented 8 years ago

It's hard to compare apples to apples because my implementation of redial on client and server doesn't double-render. Hmmm

tizmagik commented 8 years ago

We are not lazy-loading yet, but we plan to. We haven't done so yet because we're slowly rewriting/porting the app over to the React+Redux stack (it's currently mostly PHP+Backbone/jQuery). As the app grows, we will definitely start to lazy-load routes and inject reducers.

redial looks interesting, and I believe we considered it (or something similar to it) but did not like the fact that it required data fetching knowledge at the Routing level -- we wanted to keep that at the individual component level. However, avoiding the double-render is certainly a big plus; but it was a cost we were comfortable with because we knew our app rendering wasn't so heavy anyway and the cost only had to be paid for pages with such components, which we knew would only be a handful.

Curious to hear your thoughts on redux-taxi -- do you feel that the double-render is a major negative?

jaredpalmer commented 8 years ago

I spent sometime last night reading through docs and source of redux-taxi, redial, and react-resolver. Thanks for adding the new section comparing it to other libs!

Just poking around with the wappalyzer chrome extension and devtools, it seems like there are several apps that make up the NYT: homepage + sections + column (backbone+PHP), pages + events + content (prototype/jquery?), search (jquery), crosswords (didn't look), classifieds, store, blogs, myaccount, video (some client-side react, but not with react-router). If the plan is to continue to keep apps to separate and around their current size (~3-10 views), I think redux-taxi is a very smart solution and I agree that the cost of double render is relatively minimal relative to the gains. However, without knowing more about the NYT it's hard for me to fully weigh costs/benefits.

tizmagik commented 8 years ago

@jaredpalmer you had suggested something around a heuristic to skip the API call for better performance before your last edit. Do you feel like that's no longer a good solution? I thought it may have been interesting, but I don't think I fully understood how it would be used. Curious to hear your thoughts.

jaredpalmer commented 8 years ago

@tizmagik Sorry, I edited my comment after realizing that instead of having a special normalizr-specific middleware you could just transform the promise ahead of time.

// so instead of this
{
   promise: axios.get(`/api/posts/${1}`).then(res => res.data),
   normalize: {
      schema: schema.posts,
      options: mergeThing
  }
}

// you can just write this
{
   promise: axios.get(`/api/posts/${1}`).then(res => normalize(res.json, schema.posts, mergeThing))
}

Also in a previous edit I had suggested a shouldCallAPI field that bails out early . I edited that out after reading in docs that the promise middleware was just an example. I assumed you were probably already doing things like that. Anyways, you can save a ton of requests if you leverage normalizr, selectors and trackingmeta.lastFetched.

// promisemiddleware

...
function isPromise(value) {
  if (value !== null && typeof value === 'object') {
    return value && typeof value.then === 'function';
  }

  return false; 
}

export default function PromiseMiddleware({dispatch, getState}) {
    return next => action => {
        const {
          promise,
          shouldCallAPI = () => true,
          meta = {},
          ...rest,
        } = action;

       // bail quickly
        if (!shouldCallAPI(getState())) {
          return;
        }

        if (!promise) {
            return next(action);
        }

         // make sure it's really really a promise
        if (!isPromise(promise)) {
          return next(action)
        }

        const id = generateRandomId();

        next({...rest, meta, ...sequence(id, START)}); 
        return promise.then(
            payload => {
              meta.lastFetched = Date.now()  // only want to track time on success
              return next({...rest, payload, meta, ...sequence(id, DONE)})
            },
            reason => {
                let error = reason;
                // By FSA definition, payload SHOULD be an error object
                // Promises in turn don't require reason to be an Error
                if (!(error instanceof Error)) {
                    let message = reason;
                    if (typeof message !== 'string') {
                        message = 'Promise rejected with data. See error.data field.';
                    }
                    error = new Error(message);
                    error.data = reason;
                }
                next({...rest, payload: error, error: true, meta, ...sequence(id)});
                return Promise.reject(reason); // Don't break the promise chain
            }
        );
    };
}

Now you can save trips to API. (Note: Did my best attempt at some NYT logic, just be very careful about edge cases).


// example state tree
{
  posts: {
    ids: [1,2,3,4,5,6,7],
    stories: {
      1: {
          title: '..',
          body: '...',
          author: '...'
        },
      2: {
          title: '..',
          body: '...',
          author: '...'
        },
      ...
    },
    categories: {
      frontPage: {
         lastFetched: ...
         ids: [1,2,3,4,5]
      },
       sports: {
         lastFetched: ...
         ids: [4,5]
       },
       ....
    }
  }    
}

// example action
{
   shouldCallAPI: (state) => getPostsByCategory(state, category) === undefined || getPostsByCategory(state, category).length < 5 || (getPostsByCategory(state, category).length > 0 && getRecencyOfPostsByCategory(state, category) > fecha.parse('5 min ago')),
   promise: axios.get(`/api/posts?category=${category}`).then(res => normalize(res.json, schema.posts, mergeThing))
 }
tizmagik commented 8 years ago

Ah, interesting! Yes, that makes sense. We are not currently fetching article data in the app that uses this library, but will keep this optimization in mind if/when we do. Thanks for sharing!

jaredpalmer commented 8 years ago

@tizmagik Also worthing looking at is redux-entity by leland (airbnb). Apparently handles data fetching and also makes normalizr more redux friendly. Could be interesting. Have not tried it yet.

All of this made me realize that I should think about state as a database (not just an object) and that Relay may actually be easier to deal with than maintaining cache logic + normalizr + selectors + reducers.

tizmagik commented 8 years ago

@jaredpalmer Yea, FWIW, the React-based portion of our app that will be rendering articles is planned to use a GraphQL server with Relay on the server and client. Plus, some good things coming down the pipe in Relay2, we're excited about where it's heading!

jaredpalmer commented 8 years ago

@tizmagik yeah that makes sense.