Link KDS to Kolibri via webpack aliases #11446

Jaspreet-singh-1032 · 2023-10-22T18:05:30Z

Summary

Added script devserver-with-kds to run kolibri using local kds.

yarn run devserver-with-kds

By default, it will consider kds directory at same level as kolibri.
To pass different kds path use.

yarn run devserver-with-kds /path/to/kolibri-design-system

…

References

…

Reviewer guidance

…

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

Jaspreet-singh-1032 · 2023-10-22T18:08:37Z

Open to suggestions.

github-actions · 2023-10-22T19:07:40Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b10.dev0_git.7.gbbb227e8.pex
Windows Installer (EXE)	kolibri-0.16.0b10.dev0+git.7.gbbb227e8-unsigned.exe
Debian Package	kolibri_0.16.0b10.dev0+git.7.gbbb227e8-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b10.dev0+git.7.gbbb227e8-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b10.dev0+git.7.gbbb227e8-0.1.0-debug.apk
TAR file	kolibri-0.16.0b10.dev0+git.7.gbbb227e8.tar.gz
WHL file	kolibri-0.16.0b10.dev0+git.7.gbbb227e8-py2.py3-none-any.whl

MisRob · 2023-10-23T08:45:09Z

@Jaspreet-singh-1032 Great, thanks so much!

I will test it out manually. @rtibbles could I ask you to have a look at code?

MisRob · 2023-10-26T10:51:12Z

This works like a charm!

Thansk @Jaspreet-singh-1032. it will be incredibly helpful for anyone working with KDS.

I tested the following

yarn run devserver-with-kds ✅
yarn run devserver-with-kds /path/to/kolibri-design-system ✅
Run yarn run devserver-hot after yarn run devserver-with-kds to confirm that local KDS repository stops being used in favor of Kolibri node_modules KDS ❓
- I experienced a problem with my local KDS repository still being used from Kolibri when I ran yarn run devserver-hot after yarn run devserver-with-kds which is not expected behavior. However, I don't see anything in code that should cause this happen, and another try fixed it. So it could be just some weird local glitch. I would appreciate if someone else could try this locally because it's important to be able to switch back reliably. If this works for a second person, I will feel confident to proceed. @Jaspreet-singh-1032 you're welcome to give this a try but I can also ask another team member.

MisRob · 2023-10-26T10:54:09Z

package.json

@@ -25,6 +25,7 @@
    "frontend-devserver": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly {1}\" yarn:hashi-dev --",
    "app-devserver": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly {1}\" yarn:app-python-devserver yarn:hashi-dev --",
    "devserver": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly {1}\" yarn:python-devserver yarn:hashi-dev --",
+    "devserver-with-kds": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly --kds --kds-path={1}\" yarn:python-devserver yarn:hashi-dev --",


I wonder if it would make more sense to use yarn:watch-hot instead of yarn:watch? Hot reload actually worked for me even without it, but I don't understand why..In any case, I think we use the hot reload the most often.

Let's wait for @rtibbles's thoughts before making any updates though, since my understanding of kolibri tools and webpack machinery is rather poor

MisRob · 2023-10-26T11:05:02Z

After we're done with the review, I'd suggest to rebase to the release v16 branch, so that we can start using it immediately (it's optional @Jaspreet-singh-1032, I can do that myself)

rtibbles · 2023-10-27T06:58:42Z

I experienced a problem with my local KDS repository still being used from Kolibri when I ran yarn run devserver-hot after yarn run devserver-with-kds which is not expected behavior

It's possible this might be a result of webpack caching - the easiest way to test this would be to run with the new command. Stop the server, delete the webpack cache (the easiest way to do this is just to delete the node_modules folder and then reinstall with yarn), then run with the devserver-hot - if it doesn't happen with this flow, then it's the webpack cache that is causing it.

rtibbles · 2023-10-27T07:00:46Z

packages/kolibri-tools/lib/cli.js

@@ -224,6 +226,8 @@ program
    list,
    []
  )
+  .option('--kds', 'Flag to use local kds', false)


I think my only comment is that having two flags seems like one too many! I would prefer to avoid the default behaviour and just require the explicit path to KDS.

The other reason this is preferable is that in situations where kolibri-tools is being used outside of the Kolibri repo, the exact nested path that is encoded in the default may not match.

Jaspreet-singh-1032 · 2023-10-31T17:50:13Z

I experienced a problem with my local KDS repository still being used from Kolibri when I ran yarn run devserver-hot after yarn run devserver-with-kds which is not expected behavior

I see, this is due to the webpack caching. I tested it by deleting the webpack cache folder at node_modules/.cache/webpack and then It worked fine.

rtibbles · 2023-10-31T17:55:06Z

I see, this is due to the webpack caching. I tested it by deleting the webpack cache folder at node_modules/.cache/webpack and then It worked fine.

Thanks @Jaspreet-singh-1032 - I'm going to have to think on this for a minute for how we might address this without having people manually delete their webpack cache!

rtibbles · 2023-10-31T19:35:52Z

It looks like the best way to handle this would be to add an exclusion for KDS to the managedPaths option in webpack: https://webpack.js.org/configuration/other-options/#managedpaths - but only add this when the kdsPath option is active.

Note that the managedPaths should default to node_modules and then a regex to do a negative exclusion of the kdsPath - although I am not 100% confident whether the exclusion should cover the file path or the kolibri-design-system alias - will need some experimenting to make sure we are still caching most things, but not KDS!

Jaspreet-singh-1032 · 2023-11-07T15:30:17Z

Hi @rtibbles, I tried the above approach but could not make it work. I need some suggestions if there is an issue with the regex or the way I implemented it or should I try any other approach.

const defaultManagedPaths = [
    // /^(.+?[\\/]node_modules)[\\/]((?!kolibri-design-system)).*[\\/]*/,
    // /^(.*[\\/]node_modules[\\/].*kolibri-design-system[\\/].*)/,
    // /^(.+?[\\/]node_modules[\\/](?!kolibri-design-system[\\/]).+?)[\\/]/,
    // path.join(data.plugin_path, 'node_modules'),
    path.join(process.cwd(), 'node_modules'),
    // /^(.+?[\\/]node_modules[\\/](kolibri-design-system[\\/].+))[\\/]/,
    // /^(?!.*[\\/]node_modules[\\/]kolibri-design-system[\\/]).*$/,
  ]
  if (kdsPath) {
    defaultManagedPaths.push(new RegExp(`^(?!${kdsPath.replace(/\//g, '\\/')}).*`));  
  }

tnrich · 2023-11-20T23:31:35Z

@Jaspreet-singh-1032 hey, got your email on the subject, thanks for reaching out. We've never experienced the alias caching issue you mentioned. We did however switch over to using vite and use the alias functionality in a similar manner. You can see an example of that here:

https://github.com/TeselaGen/tg-oss/blob/master/vite.react.config.ts#L182

MisRob · 2023-11-21T04:30:11Z

Thanks everybody for looking into this. Just noting that Richard, who is best familiar with Webpack and this work, is out for a couple of weeks. We will come back to this.

MisRob · 2023-11-21T04:35:16Z

@tnrich Wonderful article, Thomas, thank you! And @Jaspreet-singh-1032 thanks for your perseverance and continuous contributions. This will save us plenty of time and some troubles :)

Jaspreet-singh-1032 · 2023-11-21T17:03:57Z

Thanks @tnrich for your response. I appreciate you taking the time to look into my query. I'll definitely check out the example you provided.

rtibbles · 2023-12-07T15:48:39Z

OK - I think maybe that was a bad idea, sorry it didn't work!

My next idea is instead to optionally add the kdsPath, if specified, to the buildDependencies array: https://github.com/learningequality/kolibri/blob/release-v0.16.x/packages/kolibri-tools/lib/webpack.config.base.js#L94 - it seems this is compatible with passing in a filename, so it should take a full hash of the kdsPath to check if the cache is valid or not. If not specified, then the hash should be different with it not being present.

Jaspreet-singh-1032 · 2023-12-07T16:39:25Z

Thanks, @rtibbles, I will try this approach and will share how this goes...

rtibbles · 2023-12-07T16:44:17Z

it seems this is compatible with passing in a filename

I actually meant:

it seems this is compatible with passing in a folder name

Jaspreet-singh-1032 · 2023-12-11T17:37:33Z

Hi @MisRob, I have pushed a commit, and the cache issue should be fixed now. It would be great if you could give it a try and let me know if it is working as expected.

MisRob · 2023-12-12T15:04:23Z

This is great @Jaspreet-singh-1032 and @rtibbles. I can confirm that the caching issue is gone and everything works as expected on my end.

Do you think it'd be possible to show an error when the KDS path is not provided? When you run plain yarn devserver-with-kds by mistake, the output isn't very intuitive for those who don't know you need to provide the KDS path. This is nothing blocking at all, just nice to have that we could also do as a follow up.

Jaspreet-singh-1032 · 2023-12-12T16:03:50Z

Yes, maybe we can do that. But Currently, if the kds path is not provided then it works as a regular devserver command. And It should not cause any error. because there is a condition and the code only makes effect when the KDS path is provided.

rtibbles · 2023-12-12T16:38:31Z

I think it would be fine to raise an error if the --kdsPath flag is used, but no value passed, probably less confusing than it just carrying on without saying anything.

MisRob · 2023-12-12T16:48:23Z

Currently, if the kds path is not provided then it works as a regular devserver command

It actually failed for me, I saw some errors around imports, I believe. I'd say that's actually better though when compared to it working as a regular devserver command since that'd be quite confusing (I doubt devs read the content of package.json command). Anyways, this is nothing blocking. I can open a follow-up. @Jaspreet-singh-1032 just let us know what would work best for you. We can merge this PR.

Jaspreet-singh-1032 · 2023-12-12T17:38:41Z

Sure @MisRob, we can have a follow-up.

MisRob · 2023-12-13T12:22:54Z

Alright, @Jaspreet-singh-1032. Thank you for all this work! @rtibbles I think I could retarget this to the release branch before merging, right? It'd be nice to have it available.

rtibbles · 2023-12-13T15:36:48Z

Yes, that should work, although it may require a rebase, depending on where it was branched from!

MisRob · 2023-12-13T16:59:51Z

Okay, will do

github-actions bot added the SIZE: small label Oct 22, 2023

Jaspreet-singh-1032 changed the title ~~added devserver-with-kds script with default and cli argument value~~ Link KDS to Kolibri via webpack aliases Oct 22, 2023

MisRob requested review from rtibbles and MisRob October 23, 2023 08:45

MisRob reviewed Oct 26, 2023

View reviewed changes

rtibbles reviewed Oct 27, 2023

View reviewed changes

github-actions bot added the SIZE: very small label Dec 11, 2023

MisRob approved these changes Dec 12, 2023

View reviewed changes

MisRob mentioned this pull request Dec 13, 2023

Display a meaningful error warning when KDS path is not provided when running yarn run devserver-with-kds #11632

Closed

Jaspreet-singh-1032 added 2 commits December 14, 2023 15:19

added devserver-with-kds script with default and cli argument value

58b2e17

removed default kds path and fixed cache issue

9d5aa5a

MisRob force-pushed the 10847-link-kds-to-kolibri-via-webpack-aliases branch from 9ba2b07 to 9d5aa5a Compare December 14, 2023 14:19

github-actions bot added DEV: dev-ops Continuous integration & deployment DEV: backend Python, databases, networking, filesystem... APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) labels Dec 14, 2023

MisRob changed the base branch from develop to release-v0.16.x December 14, 2023 14:20

Random change to trigger GH PR checks

bebe0cd

github-actions bot removed DEV: dev-ops Continuous integration & deployment DEV: backend Python, databases, networking, filesystem... APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) labels Dec 14, 2023

MisRob merged commit 62f79a9 into learningequality:release-v0.16.x Dec 14, 2023
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link KDS to Kolibri via webpack aliases #11446

Link KDS to Kolibri via webpack aliases #11446

Jaspreet-singh-1032 commented Oct 22, 2023

Jaspreet-singh-1032 commented Oct 22, 2023

github-actions bot commented Oct 22, 2023 •

edited

Loading

MisRob commented Oct 23, 2023

MisRob commented Oct 26, 2023

MisRob Oct 26, 2023

MisRob commented Oct 26, 2023

rtibbles commented Oct 27, 2023

rtibbles Oct 27, 2023

Jaspreet-singh-1032 commented Oct 31, 2023

rtibbles commented Oct 31, 2023

rtibbles commented Oct 31, 2023

Jaspreet-singh-1032 commented Nov 7, 2023

tnrich commented Nov 20, 2023

MisRob commented Nov 21, 2023

MisRob commented Nov 21, 2023

Jaspreet-singh-1032 commented Nov 21, 2023

rtibbles commented Dec 7, 2023

Jaspreet-singh-1032 commented Dec 7, 2023

rtibbles commented Dec 7, 2023

Jaspreet-singh-1032 commented Dec 11, 2023

MisRob commented Dec 12, 2023

Jaspreet-singh-1032 commented Dec 12, 2023

rtibbles commented Dec 12, 2023

MisRob commented Dec 12, 2023

Jaspreet-singh-1032 commented Dec 12, 2023

MisRob commented Dec 13, 2023 •

edited

Loading

rtibbles commented Dec 13, 2023

MisRob commented Dec 13, 2023

Link KDS to Kolibri via webpack aliases #11446

Link KDS to Kolibri via webpack aliases #11446

Conversation

Jaspreet-singh-1032 commented Oct 22, 2023

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

Jaspreet-singh-1032 commented Oct 22, 2023

github-actions bot commented Oct 22, 2023 • edited Loading

Build Artifacts

MisRob commented Oct 23, 2023

MisRob commented Oct 26, 2023

MisRob Oct 26, 2023

Choose a reason for hiding this comment

MisRob commented Oct 26, 2023

rtibbles commented Oct 27, 2023

rtibbles Oct 27, 2023

Choose a reason for hiding this comment

Jaspreet-singh-1032 commented Oct 31, 2023

rtibbles commented Oct 31, 2023

rtibbles commented Oct 31, 2023

Jaspreet-singh-1032 commented Nov 7, 2023

tnrich commented Nov 20, 2023

MisRob commented Nov 21, 2023

MisRob commented Nov 21, 2023

Jaspreet-singh-1032 commented Nov 21, 2023

rtibbles commented Dec 7, 2023

Jaspreet-singh-1032 commented Dec 7, 2023

rtibbles commented Dec 7, 2023

Jaspreet-singh-1032 commented Dec 11, 2023

MisRob commented Dec 12, 2023

Jaspreet-singh-1032 commented Dec 12, 2023

rtibbles commented Dec 12, 2023

MisRob commented Dec 12, 2023

Jaspreet-singh-1032 commented Dec 12, 2023

MisRob commented Dec 13, 2023 • edited Loading

rtibbles commented Dec 13, 2023

MisRob commented Dec 13, 2023

github-actions bot commented Oct 22, 2023 •

edited

Loading

MisRob commented Dec 13, 2023 •

edited

Loading