JS Public API Update #12722

rtibbles · 2024-10-17T22:33:02Z

N.B. I strongly advise you to only review this commit by commit (skipping the very large commits that move all files and update all imports) - opening the Files changed tab for all the changes here will probably crash your tab, if not your browser.

Summary

For the new API this implements, see #5488

Creates migration script that reads file imports in source code and maps them to the updated import paths - available as kolibri-tools command migrate
- Shows a warning to a user if a mapped import maps to null
- Shows a warning for jest.mocks where the module mapping is more complex, as the mocks will need to be resolved manually
Migrates every APIResource into its own module, all except TaskResource, is in kolibri-common
Migrate the new independent packages into their own packages in the kolibri packages directory
Creates a kolibri package
- migrates all public modules there
- sets exports explicitly in the package.json to prevent access to internal only modules
- adds an 'exposes' field that details the external modules the core JS API also supports
Migrates all other common modules to kolibri-common
Removes all core aliases
- Removes the use of the custom import resolver in our linting configuration, but retains it for use in a couple of other cases for now
Updates core externals to map to a spec object defined in the new kolibri package
Updates the spec export tools to introspect the kolibri package package.json to generate the apiSpec file
- Use require statements exclusively to ensure we are always exposing the full module object
- Add a special module resolution for the vue composition API plugin that managed to break this due to its janky internal module configuration

References

Fixes #5488

Reviewer guidance

The most changes here are in two commits, one that moves all the files to their new locations, and the other that updates all the import references. I would strongly recommend not viewing these commits in a browser interface.

Aside from this, absolutely nothing should have changed from a functional point of view, and all tests should be passing. If anything is awry, then that will need fixing.

There may be minor deviations from the spec, as it became a little tiresome to keep them in sync, please flag if anything looks suss, and I can either explain or rectify. Also happy to hear comments where the spec doesn't make as much sense as it might!

Lastly, this is not a finalization of the api spec - I suspect that we can finish removing the core vuex, and hence remove it from the core API completely, and if we want to break up, say, commonCoreStrings into more explicit translator modules, that also seems like a good idea.

Two other open questions I had were about the kolibri package:

For all the other new packages I created, I put everything inside a src directory inside the package, I am not sure why I didn't do the same for the kolibri package, but it would be straight forward enough to do and then update the exports field accordingly.
Is it obvious enough which things are private and which are public in this package? i.e. by using the exports field, or would it be better to try to make it more obvious by either prefixing private modules with _, or putting the api files into a special folder, and the private ones elsewhere?

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

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

github-actions · 2024-10-17T22:46:25Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0.dev0_git.20241114220415.pex
Windows Installer (EXE)	kolibri-0.18.0.dev0+git.20241114220415-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0.dev0+git.20241114220415-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0.dev0+git.20241114220415.dmg
Android Package (APK)	kolibri-0.18.0.dev0+git.20241114220415-0.1.4-debug.apk
TAR file	kolibri-0.18.0.dev0+git.20241114220415.tar.gz
WHL file	kolibri-0.18.0.dev0+git.20241114220415-py2.py3-none-any.whl

rtibbles · 2024-10-17T23:46:39Z

Switching to draft - something has gone awry in the build that I hadn't seen previously.

rtibbles · 2024-10-18T01:03:33Z

Issues resolved.

marcellamaki

I've only added a couple of comments so far and am still digesting. I haven't decided yet about my opinion on

i.e. by using the exports field, or would it be better to try to make it more obvious by either prefixing private modules with _, or putting the api files into a special folder, and the private ones elsewhere?

and will come back to that later with some thoughts.

Not blocking for this PR but I think it would be very worthwhile to update the dev docs both for accuracy and also to have some guidance for common vs. core, and in what circumstances we will make updates to the core API spec (at least as we move towards/before we get to v1.x)

marcellamaki · 2024-10-24T13:29:35Z

kolibri/core/package.json

-    "vuex": "^3.6.2",
-    "vuex-router-sync": "^5.0.0",
-    "xstate": "^4.38.3"
+    "core-js": "^3.38"


So what's happening in this commit generally "Add new base kolibri packages" is that for everything that got moved out of kolibri core, where the dependencies were required in a moved file, the package.json for that is in the plugin where the components that require those dependencies are located? Or the packages, or kolibri-common.

Yes - now this package.json only includes things that are only imported in the default frontend bundle (which is very little). Most of the imports are used either in the kolibri package or the kolibri-common package, and so are now specified there.

marcellamaki · 2024-10-24T14:12:49Z

packages/kolibri-tools/lib/webpack.config.plugin.js

+      // conditionally imports the prod or dev version of the package based on the NODE_ENV, but
+      // only exposes the common JS builds, and reassigns them to the module.exports, thereby
+      // preventing webpack from importing it consistently in both how we use it internally
+      // in the core bundle, and also then access it via externals in the plugin bundles.


I am following this comment 80% and I think I understand in broad strokes what is happening, but if I were to really explain the why, I don't think I would get it. The two phrases that I keep getting tripped up on are

only exposes the common JS builds, and reassigns them to the module.exports

and

also then access it via externals in the plugin bundles

Basically, internally, the composition api does something really janky for its exports, which breaks how we expose the 'core' modules on the default frontend bundle for the plugins to reuse without having to bundle them. This is a workaround for that.

If it makes you feel any better, once we've upgraded to Vue 2.7, we can get rid of this entirely, because we won't need the plugin any more!

packages/kolibri/apiSpec.js

bjester · 2024-10-31T17:04:41Z

On the topic of public vs private, the exports field, and prefixing _ to explicitly distinguish private code, could using the _ convention allow us to write a linting rule that ensures the exports field is kept up-to-date?

marcellamaki · 2024-10-31T17:15:22Z

the public files will end up having to do a lot of long relative path imports, and code that is used together doesn't naturally live together

Right, I hadn't really thought about that. I think prefixing is fine then. And I will reread the documentation updates, and see if there's anything else that I can think of that might be worth adding there to help.

bjester · 2024-10-31T17:54:42Z

Re: public vs private and exports

It appears this feature is also possible, according to node.js docs, where you specify null to identify what's internal.

{
  "name": "my-package",
  "exports": {
    ".": "./lib/index.js",
    "./feature/*.js": "./feature/*.js",
    "./feature/internal/*": null
  }
}

bjester · 2024-10-31T17:56:54Z

For all the other new packages I created, I put everything inside a src directory inside the package, I am not sure why I didn't do the same for the kolibri package, but it would be straight forward enough to do and then update the exports field accordingly.

Consistency always feels good and helps to clearly distinguish from package tooling or other settings, from the actual source code, so I support this

rtibbles · 2024-10-31T18:47:12Z

On the topic of public vs private, the exports field, and prefixing _ to explicitly distinguish private code, could using the _ convention allow us to write a linting rule that ensures the exports field is kept up-to-date?

Yes, I think so - and we could even have the exports field auto generated similarly to the way that the apiSpec file is autogenerated from the exports and exposes fields.

rtibbles · 2024-10-31T18:49:02Z

It appears this feature is also possible

Interesting, so technically you could do a wild card instead of feature and have it so any folder labelled with internal is not exportable.

bjester · 2024-11-01T20:00:00Z

kolibri/plugins/device/assets/src/views/__test__/RearrangeChannelsPage.spec.js

-import useUser, { useUserMock } from 'kolibri.coreVue.composables.useUser';
-import useSnackbar, { useSnackbarMock } from 'kolibri.coreVue.composables.useSnackbar';
+import useUser, { useUserMock } from 'kolibri/composables/useUser'; // eslint-disable-line
+import useSnackbar, { useSnackbarMock } from 'kolibri/composables/useSnackbar'; // eslint-disable-line


Why does eslint not like these lines?

Because the named exports only exist inside the mock, not inside the composable module itself, but because it is being mocked, they are available as an export.

bjester · 2024-11-01T20:09:46Z

kolibri/plugins/learn/package.json

@@ -3,6 +3,10 @@
  "description": "Learn Plugin for Kolibri",
  "private": true,
  "version": "0.0.1",
+  "dependencies": {
+    "deep-object-diff": "^1.1.9",


Not related to your work here, but I'm curious if lodash can accomplish what this dependency does

Yes, looking at the usage in place, it is only being used to check deep equality, so lodash's isEqual would do the job here instead.

bjester

I spot checked a bunch of files but didn't find any concerns other than those I've already brought up before this. 👍

rtibbles · 2024-11-01T22:44:30Z

Added follow up issue for resolving the src folder work here: #12780

Empty core API.

Update core externals generation. Add automatically generated API spec. Update core specific module replacements.

Remove extraneous getters now covered by the useUser composable.

…'internal'. Update the kolibri package build tools to leverage this fact to automatically generate the exports field.

indirectlylit · 2024-11-14T23:21:55Z

💥 🎉

rtibbles added this to the Kolibri 0.18: General maintenance milestone Oct 17, 2024

rtibbles marked this pull request as draft October 17, 2024 23:46

rtibbles force-pushed the public_api branch from 1967726 to f4cad9b Compare October 18, 2024 00:42

rtibbles marked this pull request as ready for review October 18, 2024 01:03

rtibbles mentioned this pull request Oct 18, 2024

Bump path-to-regexp from 1.8.0 to 1.9.0 in /kolibri/core #12719

Closed

rtibbles force-pushed the public_api branch from f4cad9b to f10c9de Compare October 18, 2024 23:45

rtibbles requested review from marcellamaki and bjester October 22, 2024 15:39

rtibbles assigned bjester and marcellamaki Oct 22, 2024

rtibbles force-pushed the public_api branch 2 times, most recently from 55eec1b to ff6377b Compare October 23, 2024 20:33

marcellamaki reviewed Oct 24, 2024

View reviewed changes

rtibbles force-pushed the public_api branch 5 times, most recently from fceba45 to 5b50afb Compare October 25, 2024 21:41

bjester reviewed Nov 1, 2024

View reviewed changes

rtibbles mentioned this pull request Nov 1, 2024

Upgrade to NodeJS 18 learningequality/kolibri-design-system#439

Open

bjester approved these changes Nov 1, 2024

View reviewed changes

rtibbles force-pushed the public_api branch 2 times, most recently from e2c890f to 687990c Compare November 1, 2024 22:39

jredrejo mentioned this pull request Nov 4, 2024

Converting machine states to version 5 of xstate learningequality/kolibri-design-system#814

Draft

8 tasks

rtibbles force-pushed the public_api branch 2 times, most recently from 19eb78e to 946c821 Compare November 6, 2024 21:50

rtibbles added 10 commits November 14, 2024 13:35

Add tooling for migrating core API imports.

fafada3

Revert stylelint-config-sass-guidelines upgrade as it causes ESM errors.

709109d

Migrate code to kolibri and kolibri-common

463d614

Add new base kolibri packages.

119dc3a

Migrate all core API import references.

e380f8b

Empty core API.

Remove custom aliases from import resolution.

660c661

Explicitly define kolibri module API.

db91ec2

Update core externals generation. Add automatically generated API spec. Update core specific module replacements.

Clean up previously unflagged import linting warnings.

3ce70aa

Remove extraneous getters now covered by the useUser composable.

Update developer documentation for core API changes.

fa80e09

Move all internal modules in the kolibri package into folders called …

e103bf9

…'internal'. Update the kolibri package build tools to leverage this fact to automatically generate the exports field.

rtibbles force-pushed the public_api branch from 946c821 to e103bf9 Compare November 14, 2024 22:04

rtibbles merged commit 143478a into learningequality:develop Nov 14, 2024
35 checks passed

rtibbles deleted the public_api branch November 14, 2024 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS Public API Update #12722

JS Public API Update #12722

rtibbles commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

rtibbles commented Oct 17, 2024

rtibbles commented Oct 18, 2024

marcellamaki left a comment •

edited

Loading

marcellamaki Oct 24, 2024

rtibbles Oct 24, 2024

marcellamaki Oct 24, 2024

rtibbles Oct 24, 2024

bjester commented Oct 31, 2024

marcellamaki commented Oct 31, 2024

bjester commented Oct 31, 2024

bjester commented Oct 31, 2024

rtibbles commented Oct 31, 2024

rtibbles commented Oct 31, 2024

bjester Nov 1, 2024

rtibbles Nov 1, 2024

bjester Nov 1, 2024

rtibbles Nov 1, 2024

bjester left a comment

rtibbles commented Nov 1, 2024

indirectlylit commented Nov 14, 2024

JS Public API Update #12722

JS Public API Update #12722

Conversation

rtibbles commented Oct 17, 2024 • edited Loading

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Oct 17, 2024 • edited Loading

rtibbles commented Oct 17, 2024

rtibbles commented Oct 18, 2024

marcellamaki left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjester commented Oct 31, 2024

marcellamaki commented Oct 31, 2024

bjester commented Oct 31, 2024

bjester commented Oct 31, 2024

rtibbles commented Oct 31, 2024

rtibbles commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjester left a comment

Choose a reason for hiding this comment

rtibbles commented Nov 1, 2024

indirectlylit commented Nov 14, 2024

rtibbles commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

marcellamaki left a comment •

edited

Loading