CaptainN / npdev-react-loadable

A Meteor port of the awesome react-loadable for webpack
MIT License
12 stars 2 forks source link

Why should meteor projects use this fork? #1

Closed Floriferous closed 4 years ago

Floriferous commented 5 years ago

Can you document what this fork does, and why it is useful for meteor apps?

CaptainN commented 5 years ago

The main reason is to gain clode-splitting (or dynamic-imports and Meteor terms), which is addressed in the readme. I'll add a note about why this fork is necessary at the top, but it's mostly about compatibility. The main react-loadable package requires webpack, and uses a few APIs (like require.resolveWeak) that don't exist in Meteor.

I submitted a PR ages ago to address that, but it was ignored. It's also nice to have a version within the atmosphere to gain some modern bundle advantages, and I was able to reduce some of the cruft that we don't need in Meteor (all that webpack bundle logic). I still have to restore a couple of API features I removed during my port (specifically, Loadable.Map).

I also simplified the SSR stuff, and rewrote the readme to show how to set that up in Meteor.

Floriferous commented 5 years ago

Yes, I'm already actively using the original package, and code-splitting in Meteor seems to work well with it. Hence my question!

CaptainN commented 5 years ago

Are you using the official package, or the one I forked a while back? If you are using the official package, are you using SSR?

I think the official version will actually work for basic use, but will not work with SSR.

This was me trying to remember why I had flagged the need to do this ages ago lol - it's for the SSR support. I also updated the core component to silence a warning from react 16.9 about componentWillMount. This fork now uses hooks, and requires React 16.8.

Floriferous commented 5 years ago

We are in fact using this package with SSR. I'm fairly sure that it works, though I'll have to check again now.

CaptainN commented 5 years ago

I think you can actually get it to work as long as you supply the webpack property/function yourself, and/or all your import functions are "resolved" (root relative, instead of relative the current file's path) - though I don't think react-loadable's babel plugin works out of the box (unless something changed in Meteor's 'require' implementation - totally possible). I'm curious to know if you did get it to work, and how.

CaptainN commented 5 years ago

@Floriferous Is it possible you are using my old fork?

Floriferous commented 5 years ago

Sorry, it took me a little while to get back to this.

I just tried running meteor with the bundle visualizer, replacing react-loadable with this meteor package, and I can report that there was no difference at all..? (it even adds 3KB extra to the bundle, might be this package's extra code)

Maybe we're not using react-loadable in a way that triggers some of the meteor-specific gotchas. We are definitely not supplying any webpack functions/configs.

Here's our reusable loadable wrapper, it doesn't do anything fancy I believe:

import { Meteor } from 'meteor/meteor';

import React from 'react';
// import Loadable from 'react-loadable';
import { Loadable } from 'meteor/npdev:react-loadable';

import { logError } from '../api/methods/index';
import LayoutError from '../components/ErrorBoundary/LayoutError';
import Loading from '../components/Loading';

const ENABLE_LOADABLE = true;

if (!ENABLE_LOADABLE && Meteor.isProduction) {
  throw new Error('ENABLE_LOADABLE should be true in production');
}

const LoadableLoading = ({ error, retry, pastDelay }) => {
  if (error) {
    return <LayoutError error={error} />;
  }

  if (pastDelay) {
    return <Loading />;
  }

  return null;
};

export default ({ req, loader, ...options }) => {
  if (!ENABLE_LOADABLE && req) {
    return req();
  }

  return Loadable({
    loading: LoadableLoading,
    delay: 200, // Hides the loading component for 200ms, to avoid flickering
    loader,
    ...options,
  });
};

And this is how we use it in the same folder as an index.js, which exports the component:

import loadable from '../utils/loadable';

export default loadable({
  req: () => require('./index'),
  loader: () => import('./index'),
});
// So if I want to use the lazy-loaded component I do 
import MyComponent from '../components/MyComponent/loadable';

// If I want to use the regular component I do 
import MyComponent from '../components/MyComponent'
CaptainN commented 5 years ago

The code for my hook shouldn't be 3KB larger than the original - maybe that's Meteor's bundle overhead? I'm not sure.

There are only 2 advantages my fork offers over the original:

  1. I ported to hooks, to avoid deprecation notices about the use of componentWillMount.
  2. This version works with a custom babel plugin to reduce the boilerplate you have to write to support SSR.

There are other forks which handle the first problem. The second problem is the main point of my fork. In your pattern though, you wouldn't see any benefit to using it, since you've abstracted out the implementation, and don't appear to be using SSR at all.

In order to support SSR, you have to supply a meteor key to the loadable, which simply wraps the require.resolve method in the current file (in the original it was a webpack key and wrapped require.resolveWeak, and also added another bundle key which we don't need for meteor).

Loadable({
    loading: LoadableLoading,
    loader: () => import('./MyComponent'),
    meteor: () => require.resolve('./MyComponent')
  });

With the babel plugin, you can simply omit the meteor key:

Loadable({
  loading: LoadableLoading,
  loader: () => import('./MyComponent')
})

I could probably do a lot more with the babel plugin (it might be nice to make it configurable too - to help with you situation for example), but this was easy to get to from the original code.

I'm curious, why do you do a direct require in development?

I've also been playing with some alternative definition APIs in my svelte-loadable patches (in the main svelte-loadable those patches are exposed as a "loader cache" which helps even without SSR to prevent flash-of-loading between components that have already loaded. Part of that for Svelte is a requirement that component loaders be pre-defined. I have thought of porting that pattern back to npdev:react-loadable as I think it's a nice separation of concerns.