learningequality / kolibri

Kolibri Learning Platform: the offline app for universal education
https://learningequality.org/kolibri/
MIT License
815 stars 692 forks source link

JS Public API Update #12722

Closed rtibbles closed 2 weeks ago

rtibbles commented 1 month ago

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

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:

  1. 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.
  2. 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

PR process

Reviewer checklist

github-actions[bot] commented 1 month ago

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 commented 1 month ago

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

rtibbles commented 1 month ago

Issues resolved.

marcellamaki commented 4 weeks ago

Follow up on the specific questions asked:

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.

I don't have a strong opinion on this, but I think for the sake of consistency, having it also in a /src package makes sense to me.

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?

I would say after reading through this a couple of times, it's not obvious. It's not hard to sort out once you've been looking around a little bit, but I think we could make it easier. Prefixing with _ seems like it would be easier to tell at a glance when looking at a specific file, but a separate folder would make more sense to me if I were coming in fresh and trying to understand the project structure overall. Which makes me think...both might be useful? Admittedly I know less about coding conventions, so was there a specific reason you suggested "either"? Or just that doing both feels unnecessary.

rtibbles commented 4 weeks ago

Or just that doing both feels unnecessary.

It felt a little unnecessary, but also both are completely doable, and would help signpost it more, if you happened to be in a deep folder structure.

The only real issue with separate public and internal folders is just that 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, so if I was only going to do one of these things, I would prefer it to be prefixing with _ so that we could keep the existing code layout.

rtibbles commented 4 weeks ago

I don't have a strong opinion on this, but I think for the sake of consistency, having it also in a /src package makes sense to me.

In that case, shall I move all the files in kolibri-common also into /src - we can do so without updating paths, by doing a wildcard export to expose the src folder as the top level.

bjester commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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.

rtibbles commented 3 weeks ago

Added follow up issue for resolving the src folder work here: https://github.com/learningequality/kolibri/issues/12780

indirectlylit commented 2 weeks ago

💥 🎉