perf: import some modules dynamically #3179

WofWca · 2023-02-28T09:46:06Z

WofWca commented

2023-02-28 09:46:06 +00:00

No description provided.

WofWca added 1 commit 2023-02-28 09:46:09 +00:00

continuous-integration/drone/pr Build is passing Details

5bf81d016f perf: import some modules dynamically

frederick commented

2023-02-28 09:52:13 +00:00

Hi WofWca!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3179-dynamic-import--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi WofWca! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3179-dynamic-import--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

dpschen commented

2023-02-28 09:55:03 +00:00

Hi @WofWca,

good idea to load some modules dynamically. Did you do some performance testing before you changed these imports? In general I'm a big fan of loading unnecessary stuff later. In this case these modules don't seem to be that large, meaning the performance gain might be minor.

Hi @WofWca, good idea to load some modules dynamically. Did you do some performance testing before you changed these imports? In general I'm a big fan of loading unnecessary stuff later. In this case these modules don't seem to be that large, meaning the performance gain might be minor.

WofWca commented

2023-02-28 09:57:55 +00:00

Nope, I didn't do any tests.
I'm just hoping to start a mainstream.

Nope, I didn't do any tests. I'm just hoping to start a mainstream.

dpschen reviewed 2023-02-28 09:58:38 +00:00

src/App.vue Outdated

						
				@ -35,8 +35,6 @@ import NoAuthWrapper from '@/components/misc/no-auth-wrapper.vue'

				import Ready from '@/components/misc/ready.vue'

				import {setLanguage} from '@/i18n'

				import AccountDeleteService from '@/services/accountDelete'

dpschen commented

2023-02-28 09:58:38 +00:00

Independent from my other comment:

We try to collect all imports in the beginning of the file even if they are dynamic.
This makes it easier to switch between a normal and a dynamic import.
You can find an example of this in the router.

Independent from my other comment: We try to collect all imports in the beginning of the file even if they are dynamic. This makes it easier to switch between a normal and a dynamic import. You can find an example of this [in the router](https://kolaente.dev/vikunja/frontend/src/branch/main/src/router/index.ts).

WofWca commented

2023-02-28 10:08:16 +00:00

Please see if the new change is ok.

WofWca added 1 commit 2023-02-28 10:07:53 +00:00

continuous-integration/drone/pr Build is passing Details

ba8ab6a4bb refactor: put dynamic imports at the top of the file

dpschen reviewed 2023-02-28 10:25:24 +00:00

src/App.vue Outdated

						
				@ -44,6 +42,9 @@ import {useBaseStore} from '@/stores/base'

				import {useColorScheme} from '@/composables/useColorScheme'

				import {useBodyClass} from '@/composables/useBodyClass'

				const importAccountDeleteService = async () => (await import('@/services/accountDelete')).default

dpschen commented

2023-02-28 10:25:24 +00:00

You can remove the async and await part to make this a bit simpler.

If you return a promise in a function it will automatically be awaitable / thenable.

You can remove the `async` and `await` part to make this a bit simpler. If you return a promise in a function it will automatically be awaitable / thenable.

WofWca commented

2023-02-28 11:01:34 +00:00

Where do I access .default then? Below, where it's used?

Where do I access `.default` then? Below, where it's used?

dpschen commented

2023-02-28 13:43:37 +00:00

Ok good point.

What about abstracting this? Then we could also add a try catch block which would make sense the moment it's async.

Ok good point. What about abstracting this? Then we could also add a try catch block which would make sense the moment it's async.

WofWca commented

2023-02-28 15:08:39 +00:00

Below, where it's used?

I think this makes sense, since it's all just about putting the imports at the top.

try catch block

I don't think it's worth it. Default is not always the only used import.

> Below, where it's used? I think this makes sense, since it's all just about putting the imports at the top. > try catch block I don't think it's worth it. Default is not always the only used import.

dpschen commented

2023-02-28 15:15:48 +00:00

How about something like safeImport and safeImportDefault (wrapping safeImport).

Vues components are always default exports and many other files use default exports in this project.

How about something like `safeImport` and `safeImportDefault` (wrapping `safeImport`). Vues components are always default exports and many other files use default exports in this project.

WofWca commented

2023-02-28 15:18:45 +00:00

What would be in the catch block? What should be returned if the fetch fails? Or are you talking about returning a Result type, like in Rust?

What would be in the catch block? What should be returned if the fetch fails? Or are you talking about returning a `Result` type, like in Rust?

dpschen commented

2023-02-28 15:35:40 +00:00

I don't write Rust but I heard that the Result type is really nice.

Originally I thought it might make sense to show a message to the user.
But we already have a block to handle unhandled rejections—currently only active in dev mode though.

What do you think would make most sense?

I don't write Rust but I heard that the `Result` type is really nice. Originally I thought it might make sense to show a message to the user. But we already have [a block to handle unhandled rejections](https://kolaente.dev/vikunja/frontend/src/branch/main/src/main.ts#L89-L94)—currently only active in dev mode though. What do you think would make most sense?

WofWca commented

2023-02-28 15:54:58 +00:00

Non-execution flow breaking error handling is nice, but I don't think it's just about this MR, e.g. the function could also throw at accountDeletionService.confirm(. I would search the internet for what people have come up with, there are some nice APIs out there I'm sure.

Also it'd be nice to first figure out what we want to happen in case something fails before we write the code.

Non-execution flow breaking error handling is nice, but I don't think it's just about this MR, e.g. the function could also throw at `accountDeletionService.confirm(`. I would search the internet for what people have come up with, there are some nice APIs out there I'm sure. Also it'd be nice to first figure out what we want to happen in case something fails before we write the code.

dpschen reviewed 2023-02-28 10:27:01 +00:00

src/App.vue Outdated

						
				@ -64,8 +65,11 @@ watch(accountDeletionConfirm, async (accountDeletionConfirm) => {

						return

					}

					const successP = importSuccess()

dpschen commented

2023-02-28 10:27:01 +00:00

Why don't you await here?

WofWca commented

2023-02-28 11:02:01 +00:00

Because it would pause the execution of the rest of the function.

👍 1

dpschen marked this conversation as resolved

dpschen commented

2023-02-28 15:09:58 +00:00

I just realised that we already load the messages in main.ts.

Taking this into consideration this pull request only dynamically imports accountDelete.ts.

Since this pull request adds an additional request at the same time I would prefer not to merge this and instead solve the core issue.

@konrad Any thoughts?

I just realised that we already [load the messages in `main.ts`](https://kolaente.dev/vikunja/frontend/src/branch/main/src/main.ts#L8). Taking this into consideration this pull request only [dynamically imports `accountDelete.ts`](https://kolaente.dev/vikunja/frontend/src/commit/dbea1f7a51f3cf5173b5f381944c4ef19ef97ec8/src/services/accountDelete.ts). Since this pull request adds an additional request at the same time I would prefer not to merge this and instead solve the core issue. @konrad Any thoughts?

WofWca commented

2023-02-28 15:13:00 +00:00

adds an additional request

Does it? I think the build script should be smart enough to not include the same module twice.

we already load the messages in main.ts.

Then we could maybe add dynamic import there as well at some point. Or maybe not.

> adds an additional request Does it? I think the build script should be smart enough to not include the same module twice. > we already load the messages in main.ts. Then we could maybe add dynamic import there as well at some point. Or maybe not.

WofWca added 1 commit 2023-02-28 15:13:33 +00:00

continuous-integration/drone/pr Build is failing Details

4f57aa6b87 refactor: swap logic around

dpschen commented

2023-02-28 15:27:02 +00:00

Does it? I think the build script should be smart enough to not include the same module twice.

Then we could maybe add dynamic import there as well at some point. Or maybe not.

If it doesn't split the import in another file what's the use for the dynamic import (seriously interested)?

> Does it? I think the build script should be smart enough to not include the same module twice. > > Then we could maybe add dynamic import there as well at some point. Or maybe not. If it doesn't split the import in another file what's the use for the dynamic import (seriously interested)?

WofWca commented

2023-02-28 15:31:58 +00:00

Runtime-wise there's no point. It's just so that it does get split if we get rid of the other static imports.

Runtime-wise there's no point. It's just so that it does get split _if_ we get rid of the other static imports.

konrad commented

2023-03-01 11:34:08 +00:00

Since this pull request adds an additional request at the same time I would prefer not to merge this and instead solve the core issue.

What's the core issue?

From what I understood, the account deletion service would only be loaded when it really is required? That sounds like a good idea to me. The effect is really minor though but I guess it still doesn't hurt?

> Since this pull request adds an additional request at the same time I would prefer not to merge this and instead solve the core issue. What's the core issue? From what I understood, the account deletion service would only be loaded when it really is required? That sounds like a good idea to me. The effect is really minor though but I guess it still doesn't hurt?

konrad commented

2024-01-16 12:08:45 +00:00

Hey @WofWca do you still want to continue with this?