Skip to content

Lazy load components #1675

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.

Sign up for GitHub

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

Merged
skjnldsv merged 1 commit into from
May 24, 2023
Merged

Lazy load components #1675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions js/viewer-components.js
View file
Open in desktop

Large diffs are not rendered by default.

28 changes: 28 additions & 0 deletions js/viewer-components.js.LICENSE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
object-assign
(c) Sindre Sorhus
@license MIT
*/

/*!
* The buffer module from node.js, for the browser.
*
* @author Feross Aboukhadijeh <[email protected]> <http://feross.org>
* @license MIT
*/

/*!
* focus-trap 7.1.0
* @license MIT, https://github.com/focus-trap/focus-trap/blob/master/LICENSE
*/

/*!
* tabbable 6.0.1
* @license MIT, https://github.com/focus-trap/tabbable/blob/master/LICENSE
*/

/*! Hammer.JS - v2.0.7 - 2016-04-22
* http://hammerjs.github.io/
*
* Copyright (c) 2016 Jorik Tangelder;
* Licensed under the MIT license */
1 change: 1 addition & 0 deletions js/viewer-components.js.map
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions js/viewer-filerobot.js
View file
Open in desktop

Large diffs are not rendered by default.

68 changes: 68 additions & 0 deletions js/viewer-filerobot.js.LICENSE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
* @license React
* react-dom.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react-is.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react-reconciler-constants.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react-reconciler.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* scheduler.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/** @license React v16.13.1
* react-is.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
1 change: 1 addition & 0 deletions js/viewer-filerobot.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions js/viewer-main.js
View file
Open in desktop

Large diffs are not rendered by default.

100 changes: 0 additions & 100 deletions js/viewer-main.js.LICENSE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,10 @@
/*
object-assign
(c) Sindre Sorhus
@license MIT
*/

/*!
* Determine if an object is a Buffer
*
* @author Feross Aboukhadijeh <https://feross.org>
* @license MIT
*/

/*!
* The buffer module from node.js, for the browser.
*
* @author Feross Aboukhadijeh <[email protected]> <http://feross.org>
* @license MIT
*/

/*!
* The buffer module from node.js, for the browser.
*
Expand Down Expand Up @@ -47,28 +34,10 @@ object-assign
* MIT Licensed
*/

/*!
* focus-trap 7.1.0
* @license MIT, https://github.com/focus-trap/focus-trap/blob/master/LICENSE
*/

/*!
* tabbable 6.0.1
* @license MIT, https://github.com/focus-trap/tabbable/blob/master/LICENSE
*/

/*! @license DOMPurify 2.4.3 | (c) Cure53 and other contributors | Released under the Apache license 2.0 and Mozilla Public License 2.0 | github.com/cure53/DOMPurify/blob/2.4.3/LICENSE */

/*! For license information please see NcModal.js.LICENSE.txt */

/*! For license information please see isFullscreen.js.LICENSE.txt */

/*! Hammer.JS - v2.0.7 - 2016-04-22
* http://hammerjs.github.io/
*
* Copyright (c) 2016 Jorik Tangelder;
* Licensed under the MIT license */

/*! https://mths.be/base64 v1.0.0 by @mathias | MIT license */

/*! https://mths.be/he v1.2.0 by @mathias | MIT license */
Expand Down Expand Up @@ -121,79 +90,10 @@ object-assign
*
*/

/**
* @license React
* react-dom.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react-is.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react-reconciler-constants.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react-reconciler.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* react.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license React
* scheduler.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @license nested-property https://github.com/cosmosio/nested-property
*
* The MIT License (MIT)
*
* Copyright (c) 2014-2020 Olivier Scherrer <[email protected]>
*/

/** @license React v16.13.1
* react-is.production.min.js
*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
2 changes: 1 addition & 1 deletion js/viewer-main.js.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions js/viewer-plyr.js
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions js/viewer-plyr.js.map
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 5 additions & 3 deletions src/components/Audios.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,18 @@
</template>

<script>
import Vue from 'vue'
import VuePlyr from '@skjnldsv/vue-plyr'
import '@skjnldsv/vue-plyr/dist/vue-plyr.css'
import logger from '../services/logger.js'

Vue.use(VuePlyr)
const VuePlyr = () => import(/* webpackChunkName: 'plyr' */'@skjnldsv/vue-plyr')

export default {
name: 'Audios',

components: {
VuePlyr,
},

computed: {
player() {
return this.$refs.plyr.player
Expand Down
10 changes: 7 additions & 3 deletions src/components/ImageEditor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@ import { basename, dirname, extname, join } from 'path'
import { emit } from '@nextcloud/event-bus'
import { showError, showSuccess } from '@nextcloud/dialogs'
import axios from '@nextcloud/axios'
import FilerobotImageEditor from 'filerobot-image-editor'

import logger from '../services/logger.js'
import translations from '../models/editorTranslations.js'

const { TABS, TOOLS } = FilerobotImageEditor
let TABS, TOOLS

export default {
name: 'ImageEditor',
Expand Down Expand Up @@ -116,7 +115,12 @@ export default {
},
},

mounted() {
async mounted() {
// Lazy load the image editor
const FilerobotImageEditor = (await import(/* webpackChunkName: 'filerobot' */'filerobot-image-editor')).default
TABS = FilerobotImageEditor.TABS
TOOLS = FilerobotImageEditor.TOOLS

this.imageEditor = new FilerobotImageEditor(
this.$refs.editor,
this.config
Expand Down
10 changes: 6 additions & 4 deletions src/components/Videos.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,23 @@
</template>

<script>
import Vue from 'vue'
import VuePlyr from '@skjnldsv/vue-plyr'
import '@skjnldsv/vue-plyr/dist/vue-plyr.css'
import logger from '../services/logger.js'
import { imagePath } from '@nextcloud/router'

const VuePlyr = () => import(/* webpackChunkName: 'plyr' */'@skjnldsv/vue-plyr')

const liveExt = ['jpg', 'jpeg', 'png']
const liveExtRegex = new RegExp(`\\.(${liveExt.join('|')})$`, 'i')
const blankVideo = imagePath('viewer', 'blank.mp4')

Vue.use(VuePlyr)

export default {
name: 'Videos',

components: {
VuePlyr,
},

computed: {
livePhoto() {
return this.fileList.find(file => {
Expand Down
11 changes: 8 additions & 3 deletions src/views/Viewer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,6 @@ import '@nextcloud/dialogs/dist/index.css'
import { showError } from '@nextcloud/dialogs'
import { emit, subscribe, unsubscribe } from '@nextcloud/event-bus'

import NcActionButton from '@nextcloud/vue/dist/Components/NcActionButton.js'
import NcActionLink from '@nextcloud/vue/dist/Components/NcActionLink.js'
import NcModal from '@nextcloud/vue/dist/Components/NcModal.js'
Comment on lines -171 to -173
Copy link
Member

@skjnldsv skjnldsv May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fond of lazy loading components that are always included. We don't gain anything but a bit more weight and network usage :)

Copy link
Member

@skjnldsv skjnldsv May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

127 0 0 1_8888_

Copy link
Member Author

@pulsejet pulsejet May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your call; I'd disagree though. The user preceivable performance is primarily due to JS (a) loading time and (b) parsing time. Splitting up the components bundle is faster on both counts. Since the bundle is prefetched and cached, the user can never know about the second request behind the scenes when the viewer is actually opened. In that sense, it's not really lazy loading, but just splitting up parsing and compilation of the critical and non-critical parts (since it all happens on the UI thread). Also the total size remains exactly the same as before, so no extra weight (note I'm looking at stat size, not parsed).

Copy link
Member

@skjnldsv skjnldsv May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the bundle is prefetched and cached, the user can never know about the second request behind the scenes when the viewer is actually opened.

That is not the case Webpack does not preload the js, it will load them on demand.
If you're on slow network, it can adds up quite a lot :)

Example given: 1.46s to wait for the viewer to load on 4G.
image

Copy link
Member Author

@pulsejet pulsejet May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you need to specify the webpackPrefetch and/or webpackPreload magic comment(s) to enable preloading. If that's not working then it's a bug maybe or prefetching is broken somehow.

https://webpack.js.org/guides/code-splitting/#prefetchingpreloading-modules

Copy link
Member Author

@pulsejet pulsejet May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Seems to work for me. If you turn on Disable Cache in the JS console then it disables prefetching; could be related to that. The above is with a cold cache in incognito.

Copy link
Member

@skjnldsv skjnldsv May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for me. If you turn on Disable Cache in the JS console then it disables prefetching; could be related to that. The above is with a cold cache in incognito.

Nope even with cache disabled 🤔
Component, plyr or editor are loaded after the viewer is opened.

I'm also ok merging this, and we can always see later how it goes 🤷
But I kind of would like a more wider discussion on async components, so we can all decide to go the same direction?

Copy link
Member

@skjnldsv skjnldsv May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be up to create a thread on https://github.com/nextcloud/standards/issues with your already impressive graphs and data to show the benefits/disadvantages and how-to of this?

Copy link
Member Author

@pulsejet pulsejet May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope even with cache disabled 🤔

I meant it wont work with cache disabled 😅. But if it wasn't disabled earlier then definitely something is off 🤔

I'm also ok merging this, and we can always see later how it goes 🤷
But I kind of would like a more wider discussion on async components, so we can all decide to go the same direction?

Sounds good 👍🏻

Would you be up to create a thread on https://github.com/nextcloud/standards/issues with your already impressive graphs and data to show the benefits/disadvantages and how-to of this?

Will do. Never knew about this repo. I think most of the overhead is coming from nextcloud-vue at this point; desperately need tree shaking in there. If that can be fixed then lazy loading components would be pointless to begin with (happy dance).

Copy link
Member

@skjnldsv skjnldsv May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. Never knew about this repo. I think most of the overhead is coming from nextcloud-vue at this point; desperately need tree shaking in there. If that can be fixed then lazy loading components would be pointless to begin with (happy dance).

This is on purpose, a bit more discrete to not become the place to discuss any thoughts. I think important topics like those should indeed be brought up there 👍

import isFullscreen from '@nextcloud/vue/dist/Mixins/isFullscreen.js'
import isMobile from '@nextcloud/vue/dist/Mixins/isMobile.js'

Expand All @@ -192,6 +189,14 @@ import Fullscreen from 'vue-material-design-icons/Fullscreen.vue'
import FullscreenExit from 'vue-material-design-icons/FullscreenExit.vue'
import Pencil from 'vue-material-design-icons/Pencil.vue'

// Dynamic loading
const NcModal = () => import(
/* webpackChunkName: 'components' */
/* webpackPrefetch: true */
'@nextcloud/vue/dist/Components/NcModal.js')
const NcActionLink = () => import(/* webpackChunkName: 'components' */'@nextcloud/vue/dist/Components/NcActionLink.js')
const NcActionButton = () => import(/* webpackChunkName: 'components' */'@nextcloud/vue/dist/Components/NcActionButton.js')

export default {
name: 'Viewer',

Expand Down
3 changes: 3 additions & 0 deletions webpack.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,7 @@ webpackConfig.plugins.push(...[
// Add proper typescript support
webpackConfig.resolve.extensions = ['.ts', '.tsx', '.js', '.jsx', '.cjs', '.vue']

// Clean dist folder
webpackConfig.output.clean = true

export default webpackConfig