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

Loadable.preload return the promise #226

Closed
wants to merge 5 commits into from

Conversation

bertho-zero
Copy link
Contributor

This PR allows to wait for the end of the preload.

@bertho-zero
Copy link
Contributor Author

I need it to block the transition during the change of location and show progress.

@theKashey
Copy link
Collaborator

There is a small but important change, I would ask you to do – "You should always return the same promise".
Right now every call to preload it would return a new promise and would call import every time. As a result it would be quite hard to await for it in a React way.

See facebook/react#14429

@bertho-zero
Copy link
Contributor Author

bertho-zero commented Feb 9, 2019

This is a function that can be executed by an event such as a click on a link, we can set a loading status before changing pages and make the page change at the end.
Like that:

  render() {
    const { children, location } = this.props;
    const { previousLocation } = this.state;

    // use a controlled <Route> to trick all descendants into
    // rendering the old location
    return <Route location={previousLocation || location} render={() => children} />;
  }

See https://github.com/bertho-zero/react-redux-universal-hot-example/blob/master/src/components/ReduxAsyncConnect/ReduxAsyncConnect.js#L42
and https://github.com/bertho-zero/react-redux-universal-hot-example/blob/master/src/utils/asyncMatchRoutes.js#L6

@bertho-zero
Copy link
Contributor Author

I use an array of routes and react-router-config to search for routes that match, I preload these components and execute a static method of each of them, it does not concern children in my case.
But I have to preload them also server side to have a correct first rendering.

@gregberge
Copy link
Owner

Hello @bertho-zero, thanks for working on it. I have some problem with that.

In my point of view, preload should not return a promise, it is like prefetch we tell to the browser "OK try to preload, but if you don't, no problem". It is not in the philosophy.

I understand your problem, but are you sure to apply the correct method? For your server-side problematic, you already have a method to do that. Loading components by yourself is not the plan.

You should use ChunkExtractorManager server-side and nothing else. What is the problem with that?

@bertho-zero
Copy link
Contributor Author

bertho-zero commented Feb 11, 2019

I use the same method as https://github.com/bertho-zero/react-redux-universal-hot-example, everything works perfectly if I add this return. The promise may not be waited, it is not an obligation. However return undefined makes it impossible to wait.

I gave all the info in the previous comments. Before rendering the route on the server or before each route change I preload the components. But for that I must be able to wait for the end of the promise.
I preload the components to see if they have a method (provideHooks of redial) and launch it while blocking the route change in the meantime. I need it server side also to not rendre the app in a state of loading.

@bertho-zero
Copy link
Contributor Author

This is the last point that makes me stuck on react-loadable

@gregberge
Copy link
Owner

I am sorry, but you should do it manually. Create a wrapper and expose a load prop that will load the component. I will not merge this in to the core. Your use-case is an edge-case and it should not be core of this library.

@gregberge gregberge closed this Feb 13, 2019
@theKashey
Copy link
Collaborator

redux-first-router is a tricky thing. Once "page change event" is dispatched - page is not changed, only a thunk for a new page is called. Once thunk resolves (it could resolve by timeout), the real transition has happened.
This is something react-router could not do, and a cornerstone of a good navigation (you may see it in iOS settings menu - delayed transition to a page).
But that's just a lyrics - to "know" that you are ready to transit to a page, you have to know that it's ready to welcome a user, ie "loaded". Thus we need awaitable preload.

To be honest - it's not so easy with "full-dynamic imports", but something should exist.

why this should be in a core - loadable rely on import keyword, or magic comments, to discover and patch original sources, thus it would be hard, or fragile to use some syntax sugar to expose importer to the client code as you proposed.

@bertho-zero
Copy link
Contributor Author

I can launch a method called preload but no way to know when this preload is complete? We have a function that launches a promise without possibility of waiting for it? And a different behavior between the client and the server for no apparent reason.

@bertho-zero
Copy link
Contributor Author

My case is not an edge-case, I will not display a page that I do not have yet and I would like to display a loading status between the beginning and the end of the loading of the page.

@gregberge
Copy link
Owner

Displaying a loading status is currently possible, Loadable Components gives you a "fallback" props that is rendered when it is loading. You are taking a wrong path, you can achieve what you want to do but not the way you want to do it.

@bertho-zero
Copy link
Contributor Author

You do not want to understand. I do not want to display fallback, but block the transition and display a bar like YouTube does. And I wish I could make a correct server side rendering.

What protects the limitation that I wanted to remove in this PR? Apart from making a universal code compatible only with the client.

Here is an example: https://react-hot-example.herokuapp.com

The code is on github and is rather appreciated, what is wrong? It's a technical choice that I can at least take with react-loadable.

Here is another exemple of what you prevent to do: https://youtube.com

@theKashey
Copy link
Collaborator

Preload/prefetch in Webpack terms is about loading some eventually.
Preload/prefetch in JavaScript terms are also about observability.
Stuff like partial hydration (React-pretendered-component) rely on the ready event from a script to take control over dead HTML.

@gregberge
Copy link
Owner

@bertho-zero an example of implementation with fallback: https://codesandbox.io/s/6l65nq9k3n.

You have to think in a Suspense way to do it, at least if you want to be future proof.

@bertho-zero
Copy link
Contributor Author

Suspense is well inside a page, but does not allow blocking the transition for the route components. It is essentially those who are affected by this change.

Your example does not correspond to what I want.

This is bad practice for you but do not have all the possible cases in mind, and I repeat my question: this limitation is supposed to protect from what?

@bitttttten
Copy link
Contributor

bitttttten commented Feb 17, 2019

I understand @bertho-zero 's need to preload the component before navigating the user away from the route.

You can do this in a library like mobx-state-router with beforeEnter/beforeExit, where the library will not allow the router to issue a navigation until these promises have resolved. Awaiting on preload from loadable components here will give a much better user experience, and I also currently do the effect @bertho-zero described using react-loadable.

But I think @neoziro 's solution is to use concurrent mode, @bertho-zero can I ask, is this the kind of effect you want? Since it is possible in concurrent mode by just using lazy and Suspense.

@theKashey
Copy link
Collaborator

It's still possible to use Suspense/LoadableState to render some component, which would lock some value in a Context onMount. But you will have to transit to the target page first.

So - no, it would not help.

@gregberge
Copy link
Owner

You should probably open a new issue on React. You will have the problem on Suspense and all data fetching very soon. Anyway, I will not fix this kind of problem in Loadable Components.

@bertho-zero
Copy link
Contributor Author

We remain on a promise that is not possible to wait...

You impose your way of doing things on everyone rather than opening up the possibilities a little without damaging anything.

This egocentric view of things is incomprehensible in the world of opensource.

@bertho-zero
Copy link
Contributor Author

bertho-zero commented Feb 22, 2019

"preload should not return a promise"
No, if I need to wait the end of preload for some components.

"You should use ChunkExtractorManager server-side and nothing else."
No, I need to have the same behavior on server and on client, it's up to me to do things right.

What do you want to "protect" us with this limitation?
What is the benefit of returning undefined rather than the promise?

I need to mix the react-router transitions and preloading as it seems to me right.

@bertho-zero
Copy link
Contributor Author

Workaround:

Step 1:

yarn add -D patch-package

Step 2:
Add an entry in scripts of the package.json:

"postinstall": "patch-package"

Step 3:
Create a patches/@loadable+component+5.6.0.patch file that contains:

diff --git a/node_modules/@loadable/component/dist/loadable.cjs.js b/node_modules/@loadable/component/dist/loadable.cjs.js
index d055987..0a055ce 100644
--- a/node_modules/@loadable/component/dist/loadable.cjs.js
+++ b/node_modules/@loadable/component/dist/loadable.cjs.js
@@ -272,11 +272,7 @@ function createLoadable(_ref) {
     });

     Loadable.preload = function (props) {
-      if (typeof window === 'undefined') {
-        throw new Error('`preload` cannot be called server-side');
-      }
-
-      ctor.requireAsync(props);
+      return ctor.requireAsync(props);
     };

     return Loadable;
diff --git a/node_modules/@loadable/component/dist/loadable.es.js b/node_modules/@loadable/component/dist/loadable.es.js
index c236921..b94489c 100644
--- a/node_modules/@loadable/component/dist/loadable.es.js
+++ b/node_modules/@loadable/component/dist/loadable.es.js
@@ -266,11 +266,7 @@ function createLoadable(_ref) {
     });

     Loadable.preload = function (props) {
-      if (typeof window === 'undefined') {
-        throw new Error('`preload` cannot be called server-side');
-      }
-
-      ctor.requireAsync(props);
+      return ctor.requireAsync(props);
     };

     return Loadable;
diff --git a/node_modules/@loadable/component/dist/loadable.js b/node_modules/@loadable/component/dist/loadable.js
index 9d19ee1..b2f4620 100644
--- a/node_modules/@loadable/component/dist/loadable.js
+++ b/node_modules/@loadable/component/dist/loadable.js
@@ -271,11 +271,7 @@
       });

       Loadable.preload = function (props) {
-        if (typeof window === 'undefined') {
-          throw new Error('`preload` cannot be called server-side');
-        }
-
-        ctor.requireAsync(props);
+        return ctor.requireAsync(props);
       };

       return Loadable;

It's sad, but I'm here for so little.

Higher up you told me about creating a Wrapper, how to create a Wrapper having access to the ctor to avoid all that?

@bitttttten
Copy link
Contributor

bitttttten commented Feb 22, 2019

It's still possible to use Suspense/LoadableState to render some component, which would lock some value in a Context onMount. But you will have to transit to the target page first.

But isn't the UX the same? Both would keep the current component/view until it's "ready".

@bitttttten
Copy link
Contributor

bitttttten commented Feb 22, 2019

The more I think about it the more I think you should not solve this by waiting for the component to load @bertho-zero .

See this talk from here on: https://youtu.be/6g3g0Q_XVb4?t=930

I think your problem is solveable with the APIs mentioned above, although I do not know there status since that talk is almost a year old.

@bertho-zero
Copy link
Contributor Author

bertho-zero commented Feb 22, 2019

I agree with you but I have been using this for years in a professional context and I do not have a plan to change everything at once immediately, I would like to be free to stay like that for the moment.

EDIT: All this is under development and I have a few more months of waiting, while I would be able to do the equivalence of that right now with this PR.
I still do not understand what's bothering with this PR, this package boasts of offering features ahead of React but also blocks some for no reason.

@gregberge
Copy link
Owner

@bertho-zero I think you should fork Loadable Components and maintain your own solution. I move in the direction of React, so this package will probably not evolve in the way you want.

@theKashey
Copy link
Collaborator

theKashey commented Feb 25, 2019

One day React will make all the loaders obsolete.
Meanwhile, I am thinking about useLoadable(() => import('file') as a solution for this problem.

const [Component, state] = useLoadable(() => import('file'));

if (state.ready) {
  return <Component />
});
return "....."; 

More about ability to use initialValue and update component when readyState changes. The "promise" itself is not quite needed.

@bitttttten
Copy link
Contributor

How would Suspense fit into that? Why not just let the not ready state be handled by Suspense?

@theKashey
Copy link
Collaborator

It's more about coding style. More than coding style exists in one moment of time. No coding style is better than another one. Never thread something new and upcoming as a silver bullet.

Lets imagine - you have code splitted a page. And now you are asked to load it. And fill with a data.

Nothing -> Loading chunk -> Loading data -> Page

You would have 2 things suspended - from loading chunk, and from loading data.

Then imagine, that I am writing this from an island in the ocean, and every -> is at least 500ms (not a joke, btw). Then - you will parallelize loading to be

Nothing -> (Loading chunk | Loading data ) -> Page

The problem - it's much harder now to "throw" a promise (which is Promise.all), as long as you have to cache it (say hello to stateful classes or hooks).

You might not to throw a joined promise, but first Component.prefetch();Data.prefetch();, and then use the "original" approach (dont forget to cache promises). And you don't have to wait for any promises - it would work out of the box - data fill eventually loaded and propagated to the state, a component will be shown.

Then add SSR, when you have to wait for promises outside of a rendering cycle. Or blocking navigation change, as seen on Suspense demo, but using a real router (react-router, RFR, react-navi...). Suddenly simple ways are no longer simple, but harder ways are simply.
It's like redux - it's huge overpower for one apps, and a savior for others.

In short - something like a promise from prefetch is needed, but there are other ways to handle it.

@bertho-zero
Copy link
Contributor Author

bertho-zero commented Feb 26, 2019

@neoziro What do you want to "protect" us with this limitation?
What is the benefit of returning undefined rather than the promise?

With React I can do that:

function lazyWithPreload(factory) {
  const Component = React.lazy(factory);
  Component.preload = factory;
  return Component;
}

const StockChart = lazyWithPreload(() => import("./StockChart"));

I move in the direction of React

... with an unexplained limitation.

React, https://github.com/theKashey/react-imported-component and https://github.com/jamiebuilds/react-loadable do it.

@gregberge
Copy link
Owner

@bertho-zero so you have three solutions that works in the way you want! That's good. I have my reasons for not returning the promise.

@bertho-zero
Copy link
Contributor Author

React is not yet complete, react-loadable is no longer maintained and I have just discovered react-imported-component.

And what are these reasons?

@bertho-zero
Copy link
Contributor Author

bertho-zero commented Feb 26, 2019

I managed to work around the problem by overloading the preload method.

// loadable.js
import loadable from '@loadable/component';

export default ( fn, options ) => {
  const Component = loadable( fn, options );

  Component.preload = fn.requireAsync || fn;

  return Component;
};

@gregberge
Copy link
Owner

@bertho-zero nice!

@bertho-zero
Copy link
Contributor Author

🎉

fivethreeo pushed a commit to fivethreeo/loadable-components that referenced this pull request Nov 16, 2022
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

Successfully merging this pull request may close these issues.

4 participants