search

nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.25k stars 4.05k forks source link

Reshaping js APIs for server and apps #15932

Closed ChristophWurst closed 4 years ago

ChristophWurst commented 5 years ago

With the many changes in how Nextcloud and Nextcloud apps' front-end is built, it's time to rethink what the APIs should look like. In this ticket I would like to draw a picture of what we have right now and where I see problems, what our APIs are about essentially and what we could do to improve the situation.

I would highly appreciate input from front-end developers, both from the point of view of server, as well as app developers :raised_hands:.

What we have right now

Our current state of JS API basically boils down to a bunch of global variables. These vary from very old (legacy) oc_xxx to the infamous OC.xxx to more structured approaches like OCA and OCP.

Internally we recently improved many aspects of the code base, but from the surface it's still problematic to rely on global vars, I think.

Problem I: documentation

Right now we have very little documentation about the front-end APIs. And if there are any, they are decoupled from the source code. As many other software projects have shown this works only for a short period of time. We should, instead, aim for having something like an autogenerated documentation from the source code. If someone changes the code, the documentation is updated automatically.

Problem II: overfetching

As we store all APIs and their implementation in global vars, every single page has to serve everything. We can't optimize for pages where only some features are used as we don't know what apps will consume. This makes the server bundle(s) considerably larger and slows down page loads, e.g. on bad internet connections or mobile devices.

The simple answer to overfetching in this context is to make the APIs async and load code only if requested. This can work, however it might worsen the UX as we always need some API some certain pages. Any async API will take slighly longer to load if not cached and therefore makes the page render deferred instead of instantly (given that data is available already). An example: let's say we had something like OC.Files that we only need for the files app and rarely for apps. We could make that an async API. All pages that don't use the API will load less code, but the files app rendering snappiness will suffer due to the async loading of the code chunk.

Problem III: dependence

While developing an app you write code against an interface that does only exist during runtime. There is no contract about what is available and what not. In order to have a somewhat decent development experience you need to load all of Nextcloud's source code to get autocompletion and access to API documentation (on hover). This works in PHPStorm but many (FLOSS) alternatives can't provide that. Someone who only develops their app, or just wants to submit a simple fix to someone else's app, they will have to rely on some documentation or similar code to see what is possible in Nextcloud. This is hacking, not engineering :wink:.

Also, during testing of apps, especially on CI, you don't always have or want to have access to the Nextcloud source code. Hence you have to mock some global variables to look or behave like the ones from Nextcloud. But again, without any clear contract. If APIs in Nextcloud change and you don't monitor the change (nobody does 100%, let's be honest) your tests will still pass while code will fail in production. The tests are therefore unreliable and thus of very little use.

Problem IV: versioning and breaking changes

If you program against one version of Nextcloud, there is no real guarantee that the used API will work in previous or future version of Nextcloud. Hence you'd have to always manually check. This can work when you start something new. But as time passes Nextcloud APIs change and you'll very unlikely notice the breakage until you or your user run into bugs.

I recently started a linter for this, which improves the situation but I still don't think this is the best solution as we can always just lint for one specific Nextcloud version and not for a range. A support range is, however, necessary for most apps on the app store. They are not just released for one version but for two to three (not checked, but taken from the experience of developing "my" apps).

Also, I do not thing that our APIs versioning has to align with the server version. Some new APIs will most likely change at a faster pace (looking at nextcloud-vue) and others won't change for like ten major version (looking at OC.generateUrl). Thus I'd be more in favor of an API versioning scheme that is semantic.

What our APIs essentially do

From all the legacy code I've recently touched I got a bit of an impression of what our current APIs do. From my understanding we have two types of APIs: small, independent helpers and mechanisms to hook into other components or register plugins.

Independent APIs

Around 90% from OC's APIs (not an accurate number but an educated guess) are simple, independent helpers. They have no dependency on the server data/state/APIs nor any other apps. These are functions like OC.generateUrl and friends. They include routing, path and l10n helpers.

APIs that communicate with server or other apps

The other, more complex share of APIs are the ones that actually have a dependency on data form server or other apps. That is plugins for the files app, sidebar plugins or any other mechanism to modify the server logic.

Proposed solution

With the analysis above and what I think is critical, I think we need some that

From the brainstorming I did with @rullzer and @skjnldsv my preferred solution would be to have new npm packages for our APIs. This would allow us to divide and conquer the madness of APIs we have and need, while making them independent of server. Apps no longer have to rely on global variables but get the (for the most part helper) code incorporated into their bundles.

I deliberately do not want to release a single package with the full API. A single package would mean more breaking changes and more critical updates in apps. Of course having like 50 packages would be a maintenance hell for use, so I'd go with a coarse-grained approach of having like a handful of packages. What I had in mind so far is nextcloud-routing for routing helpers, nextcloud-path for all the (files) path helpers, nextcloud-logging for a (new) unified logging interface that holds more context that console.xxx invocations and nextcloud-l10n for the localization parts.

In order to have a decent documentation generated I would like to give typescript a go. I've used it with nextcloud-server, the experiment that sparked many ideas for this proposal and the generated documentation is quite usable, even though it's just a quick and dirty write down to get a basic version working.

Regarding support version ranges I would try to make the APIs work with as many versions of Nextcloud as possible. And when I look at our current APIs this should be much of a challenge for the independent ones as there is close to nothing that changes from version to version. So your app will most likely use v1.x for the eternity without any breakage. And if there every is a breaking change we'll release a new major version. The package can, however, internally still work with all supported versions of Nextcloud. The critical part here is that a breaking change in one of these libraries does not mean breaking Nextcloud support but breaking the surfacing API. For anything that needs to be handled differently for certain Nextcloud versions packages should be able to just check the version it's loaded on and act accordingly.

For the APIs that communicate with server or other apps we still have to rely on global variables for the communication as I do not know any other mechanism in JavaScript. However, these internals could be completely hidden from the apps that use a package of that kind. They only work with the package's interface. They should not worry about that. And for those packages we either use what we already have or make better internal APIs. But that is irrelevant for this discussion.

In terms of maintenance overhead I'd look into sharing configuration as much as possible so that maintenance is simple and automated. This means we could create a package nextcloud-browserlist to share the common browser list configuration for Babel (also great for apps) and possibly something to share the basic webpack config. But it should all be simple to set up. We don't have to worry about that too much, as this usually becomes less of a problem after the first few iterations.

@nextcloud/javascript @nextcloud/designers please have a look and let me know if this makes sense. I'd love to improve this for the sake of having a better development experience :)

From the server point of view a solution like this would eventually give us some space for cleanups of the oc_xxx, OC, OCP and OCA madness.

I know it's not easy to change this and it's going to be a long process but I think we can make the front-end pretty awesome :rocket:

My project board for the ticket once I have some specific tasks to work on: https://github.com/orgs/nextcloud/projects/30

skjnldsv commented 5 years ago

Lots of very solid points and ideas! I agree with your choices and feels like this is a great enhancement!

Questions

  1. If I create a simple app with a single script which I write in pure js because this is the cleaner and easier way. How can I use the oc routing or others api features?
  2. How do we manage the use of a nextcloud-xxx library that have nc16 methods on an nc15 installation (if my app is compatible with 15+16 for example) ?
ChristophWurst commented 5 years ago

If I create a simple app with a single script which I write in pure js because this is the cleaner and easier way. How can I use the oc routing or others api features?

That is a limitation of this approach. Those yolo apps can still use the legacy APIs as they won't die anytime soon. But in general we should make the webpack setup as easy as possible for apps (with generators, maybe?) so everyone can benefit from the new packages and their own dependencies.

How do we manage the use of a nextcloud-xxx library that have nc16 methods on an nc15 installation (if my app is compatible with 15+16 for example) ?

What do you mean by methods? The nextcloud-xxx package will be as independent from server as possible. In most cases there is no difference when used on 15 or 16. But if there is, the package can just check the version (it's stored in the config somewhere IIRC) and then either follow one or another path. The app should not notice a difference as there is only a single API it interacts with. The magic happens in the package's API implementation.

skjnldsv commented 5 years ago

But in general we should make the webpack setup as easy as possible for apps (with generators, maybe?) so everyone can benefit from the new packages and their own dependencies.

That is where I disagree. If I add a setting section to my app and I want a simple dialog to confirm a change, I do not need a full webpack setup. My suggestion is to keep having those local api but using the library?

What do you mean by methods? The nextcloud-xxx package will be as independent from server as possible. In most cases there is no difference when used on 15 or 16.

Well, for routing (e.g) this make sense as they're just independent wrapper, but I don't know. Theoretical example with OC.getUser

Not sure if i'm clear :see_no_evil:

ChristophWurst commented 5 years ago

That is where I disagree. If I add a setting section to my app and I want a simple dialog to confirm a change, I do not need a full webpack setup. My suggestion is to keep having those local api but using the library?

That is a possibility. But it won't solve our problem of shipping so much code on every page (overfetching). We could offer a OC.getRouting (and similar for the others) abstraction to load our main API packages async if apps want to use them. but that also means each packages has to be some kind of object that is re-exportable. Or we have to make it one in server.

Now the nextcloud-user library was providing a getUser method. WOuld we change the library method as well? Or change the endpoint to which the library fetch the data? (check if OC.getUserInfo, then OC.getUser) so that the transition is silent to the dev?

The way I see it is that neither the app nor the package should use OC.getUser if its implementation has no dependency on other components. Then you can use it with any version of Nextcloud. If the API depends on some route/API/whatever, then it's more tricky. Then the package would have to introduce a breaking change if it's not possible to work around the limitation on previous Nextcloud versions. If there is some kind of fallback possible the package API can stay the same but the internals have to distinguish between Nextcloud version in use and run different code paths.

Is there any case I'm overlooking?

skjnldsv commented 5 years ago

That is a possibility. But it won't solve our problem of shipping so much code on every page (overfetching).

Yes, that is an issue :/ What about an import function that load async? That way we could emulate an import from any js file? Not sure if possible :thinking:

ChristophWurst commented 5 years ago

What about an import function that load async? That way we could emulate an import from any js file? Not sure if possible thinking

That is exactly what I outlined in my previous reply and with

The simple answer to overfetching in this context is to make the APIs async and load code only if requested. This can work, however it might worsen the UX as we always need some API some certain pages. Any async API will take slighly longer to load if not cached and therefore makes the page render deferred instead of instantly (given that data is available already). An example: let's say we had something like OC.Files that we only need for the files app and rarely for apps. We could make that an async API. All pages that don't use the API will load less code, but the files app rendering snappiness will suffer due to the async loading of the code chunk.

jancborchardt commented 5 years ago

Generally sounds good! Especially if it improves the developer experience and ease to start fetching stuff from other apps. Working on Keep or Sweep for example it seems impossible to just get Contacts or other data without being a knowledgeable programmer.

I can probably not give too much feedback here, but 2 points which came to mind:

ChristophWurst commented 5 years ago

We should also think about external apps (not only apps inside Nextcloud)

Totally agree on that. But it's not really related to this discussion. Most of the helpers I want to package in the discussed packages are for internal use. Anything external will be found in (an) additional package(s).

When working with APIs recently, the ones which used GraphQL were suuuper easy.

You're talking about web API design. This ticket is about solely about JS APIs. What they use internally for the server communication is not relevant to the user of the API package. And in general implementing GraphQL is not a trivial task without a decent framework that helps you achieve it. I do not think we can integrate it into Nextcloud easily. Moreover the most important API for Nextcloud is DAV, which is a universal standard that works with so many clients. But that has to be discussed in a ticket of its own.

ChristophWurst commented 5 years ago

We could offer a OC.getRouting (and similar for the others) abstraction to load our main API packages async if apps want to use them. but that also means each packages has to be some kind of object that is re-exportable. Or we have to make it one in server.

My current roadmap is

ChristophWurst commented 5 years ago

Progress

FYI this works very well so far. The repos are minimal (both in size and configuration) and the package are nice to use in Mail (where I test them right now).

rullzer commented 5 years ago

As discussed I like it. I think moving this forward would be great for apps and our internal code.

MorrisJobke commented 5 years ago

Sounds really good sand the choice for TypeScript is good as we see how much it makes stuff in the backend better with typings. 👍

I still need to raise one question: How does it make thing smaller in the long run?

Because now it would ship the code of OC.L10n for example in every app and for the files app this would mean

Or did I miss something?

Looking at https://github.com/ChristophWurst/nextcloud-l10n/blob/master/lib/index.ts it still calls the global methods, which then only would duplicate the interface code but then still the problem with potential overmatching is still there. 🤔

ChristophWurst commented 5 years ago

How does it make thing smaller in the long run?

Smaller? If you're talking about the amount of code loaded (in bytes) then in the long run apps will load only the code they need thanks to tree shaking and other techniques in bundlers. On the other hand the server code can shrink as it won't expose all the APIs and modules anymore.

Yes, right now most nextcloud-* packages are just proxies for the old APIs. The goal is to establish a decent API apps can work with. The internals can change later and the idea is to inline as much as possible over time.

Yes, there would be a bit of duplication with often used modules like nextcloud-l10n but in the end the code is minimal and therefore I don't think it would cause a significant increase in code size. If we take the current core/src/OC/l10n then it's 12kB in size. Ran through a bundler it will be ~8kB (test with an online minifier). If that is loaded 8 times (adding some room to your estimate) it would be 64kB of code. IMO this is okay. Since most apps don't even use all the code inside that module the actual sum is likely to be smaller after tree shaking.

MorrisJobke commented 5 years ago

We should keep an eye on that, but it’s true - if we try to remove as much code that is always loaded to only provide the often not used APIs then we already have gained a lot. And decluttered our JS code. 👍

ChristophWurst commented 5 years ago

Small update on the packages since the recent rename.

The nextcloud- packages were experiments. Now that we use them in many places and they seem to work, I renamed to @nextcloud/* and re-published under the new name. Here is a list of the packages:

Please start using the new packages. Migrate from the old ones if you use them in your apps.

Docs for the packages will follow shortly. Your IDE or editor should, however, be clever enough to give you decent auto-completion when using the packages as they are all typed with Typescript.

ChristophWurst commented 5 years ago

Also dropping this here in case someone has more experience with this (I have none).

I want to have docs for the packages, obviously. And there is typedoc. But it generates docs for just one version of one packages at a time. It would be fabulous if it, or another tool, could

Does anybody have a clue or is better with duckduckgoing this? Couldn't find anything helpful when I checked.

ChristophWurst commented 4 years ago

Hello again!

Today I finalized the v1.0 of most packages. This means they now all have a somewhat complete documentation generated from the typescript code. The docs are all pushed to github pages automatically when a release tag is pushed.

I'm closing this ticket thus. Any further concerns, bugs and feature requests shall go into the repos of the individual packages. Happy hacking!