Use consistent KDS imports across the whole Kolibri

[MisRob]

There are currently two ways we import KDS (Kolibri Design System) logic (e.g. a component, a mixin, a composable) to Kolibri:

(1) Via kolibri.coreVue

First, a piece of KDS logic, in this example UiIconButton, is imported from kolibri-design-system and registered in apiSpec.js

kolibri/kolibri/core/assets/src/core-app/apiSpec.js

Line 22 in 4de6c2f

import UiIconButton from 'kolibri-design-system/lib/keen/UiIconButton'; // temp hack

kolibri/kolibri/core/assets/src/core-app/apiSpec.js

Line 192 in 4de6c2f

UiIconButton,

This makes it available in the whole Kolibri for import from kolibri.coreVue:

kolibri/kolibri/plugins/learn/assets/src/views/QuizRenderer/index.vue

Line 160 in 4de6c2f

import UiIconButton from 'kolibri.coreVue.components.UiIconButton';

(2) Directly

The other approach imports a piece of KDS logic, in this example UiAlert, to a Kolibri component directly from the kolibri-design-system package:

kolibri/kolibri/plugins/learn/assets/src/views/QuizRenderer/index.vue

Line 159 in 4de6c2f

import UiAlert from 'kolibri-design-system/lib/keen/UiAlert';

Goal

The goal of this issue is to use approach (2) everywhere. Using direct imports from KDS will ensure consistency and make it clear where code is being imported from.

Acceptance criteria

• All JavaScript coreVue imports for KDS are replaced by direct imports in all Kolibri files
• All related KDS logic is removed from apiSpec.js

Out of scope

Note that we also use direct KDS styles imports, for example @import '~kolibri-design-system/lib/styles/definitions';. Dealing with these Sass imports is not in the scope of this issue and they can stay as are.

Guidance

1. Search the codebase for api spec kolibri.coreVue imports
2. If an imported piece of KDS logic is registered in apiSpec.js, remove it.
3. Replace the coreVue import with the direct import

[MisRob]

This issues is to target the develop branch

[ShivangRawat30]

I would like to work on this issue.

[MisRob]

Hi @ShivangRawat30, thank you, I'm assigning you. Good luck and let us know if you needed anything.

[nucleogenesis]

I think maybe we should consider strongly replacing all uses of vendored code with KDS components directly. This would be a substantial amount of work, however, so could be created as a separate issue to be dealt with down the road as it would involve updating many places in Kolibri to import/use a new component.

[MisRob]

@nucleogenesis Yes, definitely. Even though I used vendored components as an example, this issue is more general and includes imports of regular KDS components as well. You can find cases where the same logic is imported in two different ways.

[MisRob]

@nucleogenesis And we'll be most likely tweaking location of composables one way or another too and those are regular KDS paths. So this will help in that case as well.

[MisRob]

@nucleogenesis I'm more than up to to not be reaching to vendored files directly eventually. Not sure what was the motivation for that.

[PR4NJ41]

@MisRob If No One is working on this issue anymore can i work on it ?? Please assign it to me and let me know. Thank You

[AlexVelezLl]

Hi @ShivangRawat30 are you still working on this issue? If there is anything we can help you with, let us know.

[PR4NJ41]

@AlexVelezLl @MisRob Could you please provide Slack Channel link where i can discuss GSOC related queries ?

[AlexVelezLl]

Hi @PR4NJ41! Sorry, we don't have a slack channel for GSoC 2024 yet, but please stay tuned for when we have everything ready =).

In the meantime, you can open new GH Discussions or use this discussion for anything related to Kolibri.

[ShivangRawat30]

@AlexVelezLl I am currently working on an other issue, you can assign the issue to someone else in the mean time.

[AlexVelezLl]

Thanks @ShivangRawat30! @PR4NJ41 I just assigned it to you 🙌

[PR4NJ41]

@AlexVelezLl Thanks for assigning me the issue. Just to confirm, i have to work on the develop branch, am i right?

[AlexVelezLl]

Hi @PR4NJ41! yes, you should work on they develop branch.

[PR4NJ41]

Hi @AlexVelezLl, I found 2 components named UiToolbar

1. import UiToolbar from '../views/KeenUiToolbar.vue';
2. import UiToolbar from 'kolibri-design-system/lib/keen/UiToolbar';

The 1st one was already present in apiSpec.js

what should i do with the 2nd one?

[PR4NJ41]

This is in apiSpec.js file

kolibri/kolibri/core/assets/src/core-app/apiSpec.js

Line 127 in 4de6c2f

import UiToolbar from '../views/KeenUiToolbar.vue';

This is the other file.

kolibri/kolibri/core/assets/src/views/KeenUiToolbar.vue

Line 3 in 4de6c2f

import UiToolbar from 'kolibri-design-system/lib/keen/UiToolbar';

[AlexVelezLl]

Hi @PR4NJ41. Could you try replacing the places where the UiToolbar from views/KeenUiToolbar.vue is used to using the one from kolibri-design-system/lib/keen/UiToolbar? You can see if they have the same props and if they work the same, then you can replace them, and prefer to use the one from kolibri-design-system/lib/keen/UiToolbar.

If they are incompatible, let's leave the views/KeenUiToolbar.vue in the apiSpec for the moment, and we can later open a follow up issue to unify both of them.

[PR4NJ41]

Hi @AlexVelezLl, I replaced it.

@MisRob @AlexVelezLl I raised a PR please review it. Thank you

[rtibbles]

Sorry, I totally missed the content of this issue. This is entirely the wrong direction of travel, and we should be removing references to KDS in the core API spec, not adding them.

If import paths update in KDS that is a breaking change that should be documented, and then can quickly be addressed in the upgrade in Kolibri with a global find replace.

Apologies for the delay here, but I think I had seen the title and assumed it was about switching to direct imports from KDS and not this away around.

[PR4NJ41]

@rtibbles So do i have to remove Kolibri.coreVue.. used in files ?

[PR4NJ41]

Also, I ran yarn run build and yarn run test they were running fine.

[rtibbles]

Hi @PR4NJ41 - I think @MisRob and I need to discuss this further before I suggest next steps, and we are both out of the office until January. We will follow up then.

[PR4NJ41]

Ok, Thank You. I will try to fix other issues.

[indiansamaltman]

please notify when you create a slack channel @AlexVelezLl

[MisRob]

@rtibbles My main concern is consistency and am not very opinionated which direction we choose. This issue is a result of discussion with @AlexVelezLl during one of the reviews, where he mentioned that using core can prevent from replacing many imports in case we change them in KDS. I though that was a good argument and therefore leaned towards that direction.

However at that time, I didn't know of #5488 and the current organization seemed to indicate that it's a common practice to put KDS components to core. I assume that #5488 is the main reason you suggest taking the opposite approach? If that's so, I think it makes sense.

[rtibbles]

Yes, when I saw the title of the issue I had assumed it was just consistently not using the core imports, rather than the other way around!

#5488 is a response to the tendency towards extreme bloating of the core API that we have seen - KDS imports are a little bit different, inasmuch as we do bundle all of the KComponents into the core through the use of the plugin and global registration, but just importing directly from KDS means that we don't have to maintain as much in the core API, and can be easily updated with a find and replace, even if import paths change.

[rtibbles]

I have updated the issue to reflect the opposite direction here. @PR4NJ41, please feel free to resume this work if you are able!

[MisRob]

Okay, thanks for taking care to update the issue, @rtibbles.

@PR4NJ41 apologies for confusion and thanks for all your work.

[PR4NJ41]

@MisRob @rtibbles
Please can you confirm which approach i have to follow now (Please refer to the Topmost part):

(1) Via kolibri.coreVue
(2) Directly

[PR4NJ41]

If we are using the 2nd approach then do i have to remove all imports which are using "kolibri.coreVue" or only certain ones.

[rtibbles]

Directly, so any imports for modules that we could import directly from KDS should be imported directly from KDS. Note that there are lots of other "kolibri.coreVue" imports that are not for KDS imports, so you can leave these as they are.

[vaibhav-rm]

I would like to help with this issue.

[PR4NJ41]

@vaibhav-rm I am already working on this

[vaibhav-rm]

@vaibhav-rm I am already working on this

Okie, Let me know If I could be of any help. Good luck.

[PR4NJ41]

@MisRob @rtibbles I've just raised another PR. I used yarn run build and yarn run test for testing.

Please review the changes. I'm open to make further improvements if needed.

Thank you!

[MisRob]

Thank you @PR4NJ41, we appreciate your flexibility and patience

[Abhinav1148]

@rtibbles, @MisRob, I've raised a PR for this issue. Please review it and please let me know if any changes are needed.
Thank you!!

[MisRob]

I'm sorry @Abhinav1148, but this has been already resolved here #11742 and will soon be merged. Please always check if there's already an asignee in an issue. If yes, than it's already taken. If no, send us a message a first and wait until we assign you.

[Abhinav1148]

I'm sorry @MisRob for this mistake, I'll take care of that from now onwards.

[MisRob]

No problem, I feel sorry this was your first experience! I just posted more information to your pull request :)

[rtibbles]

Fixed in #11742