-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
App load tweaks #12869
App load tweaks #12869
Conversation
… time Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
…vector-im/riot-web into t3chguy/app_load_tweaks
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
…vector-im/riot-web into t3chguy/app_load_tweaks
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, just one suggestion. I'm not entirely sure how code-splitting will benefit us though, as I suspect that the big bundles will still change every release? What are the chunks we want to eventually end up with? Perhaps this is related to Travis' Jitsi work? I'm not really aware of the details of that.
Would also be great to document this, but I can imagine you've already got that planned in the follow-up PRs.
configSyntaxError = true; | ||
configJson = {}; // to prevent errors between here and loading CSS for the error box | ||
} | ||
return e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you return rather than throw the error here to receive the error without a try/catch in app.js:190
. Returning errors from a promise is rather non-idiomatic though. Can I suggest to do it the more straight-forward way and throw and catch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that's exactly what this code did previously as per Travis' doing except it was returning an error and a boolean
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, missed that it was already there. Not sure how returning rather than throwing will allow first paint sooner but not too big of a deal, I'm fine either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented that in the wrong place
Possibly but reusable chunks between entrypoints and also just the sanity of loading, so we can not import code which uses things like localStorage until we have ran modernizr etc. |
|
I see, ok 👍 |
I guess I'm just a bit worried of further complicating our webpack setup, but given we already have async components, this doesn't add much complexity? Wdyt? |
It doesn't add any webpack-level complexity, its just the use of dynamic-imports and webpack optimizes where the splits happen |
This PR pulls some of the simpler stuff out of my larger changeset and paves the work for code-splitting by grouping loading code better. It also converts some stuff to TypeScript. This set of changes does not yet impact bundle size.
Fundamentally (but not yet split):
The app will later be as preloaded as possible to make the best use of HTTP/2 fetch parallelism.