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

Allow Modal to be used with async-loaded components #618

Merged
merged 1 commit into from Jan 19, 2017
Merged

Conversation

@richvdh
Copy link
Member

richvdh commented Jan 16, 2017

Here's a vaguely interesting hack: with very few lines of code, it's possible to delay loading of bits of riot-web until they are actually needed. Here is a demonstration of how we might do it for modal dialogs (which seem to be obvious candidates for factoring out in this way).

However, this will break existing users when we deploy a new version (they might be looking for 1.6bfef606b3e11954030e.js, but the new file could be called 1.af6f5457910a37776515.js, so they'll get a 404). The only plausible way to make this work would be to keep old versions of the submodules available.

... so this PR isn't ready to land until we solve that problem.

Add Modal.createDialogAsync, which can be used to display asynchronously-loaded
React components. Also make EncryptedEventDialog use it as a handy
demonstration.
@richvdh richvdh force-pushed the rav/async_dialog branch from 702e1b1 to ac22803 Jan 16, 2017
@richvdh

This comment has been minimized.

Copy link
Member Author

richvdh commented Jan 16, 2017

@richvdh

This comment has been minimized.

Copy link
Member Author

richvdh commented Jan 16, 2017

vector-im/riot-web#2961 is a step on the way to solving that.

@richvdh

This comment has been minimized.

Copy link
Member Author

richvdh commented Jan 17, 2017

vector-im/riot-web#2969 was another part of the puzzle. We still need a new script for updating riot.im/app, but that seems to work in principle. Which means this PR is viable after all.


componentWillMount: function() {
this._unmounted = false;
this.props.loader((e) => {

This comment has been minimized.

Copy link
@dbkr

dbkr Jan 18, 2017

Member

Is there a reason for callbacks rather than promises here, ooi?

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 18, 2017

Author Member

just that require (or require.ensure, for that matter) takes a callback, rather than returning a promise. We can't easily wrap require with something that returns a promise, because that breaks webpack's static analysis. So if loader was expected to be a Promise, we would have to add some nasty boilerplate everywhere we loaded a component asynchronously.

IOW: currently we can do:

createDialogAsync((cb) => {require(['foo'], cb)});

If instead we had to pass a promise, this would become:

d = q.defer(); 
require(['foo'], (m) => {d.resolve(m)});
createDialogAsync(d.promise);

This comment has been minimized.

Copy link
@dbkr

dbkr Jan 18, 2017

Member

Fair enough, if it makes it simpler then callbacks ftw.

var event = this.props.mxEvent;

Modal.createDialog(EncryptedEventDialog, {
Modal.createDialogAsync((cb) => {
require(['../../../async-components/views/dialogs/EncryptedEventDialog'], cb)

This comment has been minimized.

Copy link
@dbkr

dbkr Jan 18, 2017

Member

I wonder whether it would be better to use the CommonJS require.ensure here, as we've used CommonJS elsewhere, rather than introducing AMD syntax (https://webpack.github.io/docs/code-splitting.html) (also require.ensure seems to me a little more obvious what it might be doing, rather than calling require with an array and a callback versus a string).

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 18, 2017

Author Member

agreed, though where have we used commonjs elsewhere? (And it's the callback that makes the difference between the two types of require, rather than the arrayness, but yes)

will make it so

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 18, 2017

Author Member

oh, except that conflicts with my previous point: using the CommonJS syntax would be considerably more verbose, as there is no longer a simple way to pass the component into createDialogAsync.

This comment has been minimized.

Copy link
@dbkr

dbkr Jan 18, 2017

Member

Well, we use straight 'require' which is CommonJS, although of course now we're moving to ES6 import syntax, but webpack doesn't support this for async loading. And yeah, it does seem to make it quite a chunk more verbose.

@dbkr dbkr assigned richvdh and unassigned dbkr Jan 18, 2017
@dbkr
dbkr approved these changes Jan 18, 2017
@richvdh richvdh merged commit ba2460a into develop Jan 19, 2017
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
default Build finished.
Details
@richvdh richvdh deleted the rav/async_dialog branch Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.