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

Code split out everything that is not needed until after user interaction #839

Open
lencioni opened this issue Nov 13, 2017 · 10 comments
Open

Comments

@lencioni
Copy link
Contributor

The way we are using react-dates at Airbnb, we often see it appear in our bundles with a lot of added filesize. One recent bundle size analysis I did clocks react-dates in at ~118 KiB!

screen shot 2017-11-13 at 9 39 09 am

Although some of the recent work we've done (e.g. #791) should help a little, I think there are more opportunities to improve this aspect of performance for this package.

Since most of these components are never needed for the initial render, I would really like to be able to either easily compose parts of react-dates so that I can code split out anything that won't be visible until after a user interaction, or be able to pull in a version of react-dates that has this code splitting built in.

I think if we are able to land something like this, we could dramatically reduce the bundle size impact of react-dates, which would be a really great win for everyone!

This is spiritually similar to #286, but approached from the bundle size side of things.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2017

Can you elaborate on how you think we could generically do this?

Requiring that a bunch of components be manually injected isn't super viable imo; using env vars wouldn't really apply here; we could specify browser entry points but then that wouldn't help bundle size, just server renders.

@lencioni
Copy link
Contributor Author

I'm just sorta thinking out loud here, and I'm not super familiar with how this package is architected, so I may be way off. I'm also not really sure what might be best practices for adding code splitting to third-party components out of the box, so maybe there's a better solution.

But, looking at SingleDatePicker for example, what if we took everything that was rendered by this function

https://github.com/airbnb/react-dates/blob/ed4d06911d3f18c7164b165939713af266f3289d/src/components/SingleDatePicker.jsx#L324-L433

and moved it into its own component that was passed in as children that are conditionally rendered. We could keep all of the props automatically wired up by using clone element. That way, the API for using this component would go from something like this:

<SingleDatePicker
  phrases={phrases}
  onDateChange={this.handleDateChange}
  onFocusChange={this.handleFocusChange}
/>

to something like this:

<SingleDatePicker
  phrases={phrases}
  onDateChange={this.handleDateChange}
  onFocusChange={this.handleFocusChange}
>
  <DayPickerSingleDateController />
</SingleDatePicker>

That would give consumers control over the import of DayPickerSingleDateController which would allow it to be code split out, e.g. via react-loadable or something like it.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2017

Hmm, that seems reasonable but also a pretty big breaking change. Would moving it into a component, alone, help? You could alias it out in your bundler without requiring an API change.

@lencioni
Copy link
Contributor Author

This could probably be done without a breaking change actually, if we moved most of e.g. SingleDatePicker into a new file, split it up in the way I describe above, and then change SingleDatePicker to import and compose both of these things and pass down all props. Then, consumers who want to use the split up version would simply stop importing SingleDatePicker, and instead import the two separate pieces and compose them together in their app however they want.

@ljharb
Copy link
Member

ljharb commented Nov 13, 2017

That seems reasonable; I'm a fan anyways of more things being split out into separate components.

@lencioni
Copy link
Contributor Author

@ljharb Can you think of good names for these components?

@ljharb
Copy link
Member

ljharb commented Nov 13, 2017

¯\_(ツ)_/¯

@Magellol
Copy link

On a sidenote, I think moment and its locales are definitely also contributing to the size. I was suprised as I've tried react-dates yesterday and even cutting out the locales through webpack didn't help much. I think there were some dicussions around having the possiblity to choose the date library being used, date-fns was one suggestion. Maybe trying to push forwards the idea of replacing moment might be interesting.

@majapw
Copy link
Collaborator

majapw commented Nov 28, 2017

I'm just still a bit confused as to how the proposed rearchitecting is all that different from what's happening now (e.g. splitting the SDP into multiple component). The SDP (and DRP as well) is basically just a wire between the inputs and the calendar dropdown. The renderDayPicker method you highlighted basically just wraps the DayPickerSingleDateController so as to absolutely position it appropriately. I don't know that it makes sense to split up those top level components more.

As for passing in <DayPickerSingleDateController /> or I guess the inputs as children, I'm just not really sure how that would... work. It seems fairly convoluted and hard to ... express exactly what the component is expecting there. Perhaps if you could provide more implementation details on what you're imagining @lencioni I could have a better understanding?

@Magellol for replacing moment with something lighter, I've thought a bit about this (although I'm still not sure what would be the best way to approach the implementation). Essentially, a lot of the moment functionality is abstracted away in these helper methods (isSameDay, isBeforeDay, getCalendarMonthWeeks). If we could provide some sort of interface that the consumer could implement in whatever way they wanted (with moment or date-fns) as the default provided options, that might be interesting. I haven't really had the bandwidth to devote much thought to this tho since we use moment internally for other things as well.

@lencioni
Copy link
Contributor Author

lencioni commented Dec 4, 2017

@majapw The goal is to allow the imports to be separated out cleanly enough with minimal API changes, so that people can use code-splitting features in their bundler (e.g. webpack) to avoid putting most of the code from this package in their main bundle.

In the implementation, everything would be mostly the same, except for renderDayPicker. Instead of importing and rendering DayPickerSingleDateController, it would expect that component to be passed in via a prop. This dependency injection allows us to hand control over to the consumer of how and when that code is imported, which enables the ability for all of that code to be split out.

I think there are two options here. One would be passing in a reference to the component class as a prop, like this:

<SingleDatePicker
  phrases={phrases}
  onDateChange={this.handleDateChange}
  onFocusChange={this.handleFocusChange}
  controllerComponent={DayPickerSingleDateController}
/>

Then you simply remove the import from SingleDatePicker and render it from the prop in renderDayPicker.

Alternatively, you could pass it in as JSX either in a prop or as children (as in my example above):

<SingleDatePicker
  phrases={phrases}
  onDateChange={this.handleDateChange}
  onFocusChange={this.handleFocusChange}
>
  <DayPickerSingleDateController />
</SingleDatePicker>

and then you remove the import and change renderDayPicker to use cloneElement to add all of the props it needs.

Either of these would allow consumers to create an async wrapper around the controller component for code splitting purposes, and pass that in instead, e.g.

<SingleDatePicker
  phrases={phrases}
  onDateChange={this.handleDateChange}
  onFocusChange={this.handleFocusChange}
  controllerComponent={LoadableDayPickerSingleDateController}
/>

And the code-split wrapper could look like something this:

import Loadable from 'react-loadable';
import Loading from './my-loading-component';

export default Loadable({
  loader: () => import('react-dates/path/to/DayPickerSingleDateController'),
  loading: Loading,
});

Does this help make sense?

@lencioni lencioni mentioned this issue Jan 25, 2018
4 tasks
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

4 participants