Closed max-nextcloud closed 1 year ago
I think this could be beneficial for other apps too. Just as a thought experiment... what if viewer provided a package that allowed
import { registerHandler } from '@nextcloud/viewer'
It could still use OCA.Viewer
underneath. But it would easily allow apps using this api to confirm that the package actually exports the registerHandler
function.
I'd find it desirable to develop this package inside the viewer repo so that changes to the package and the app can be made at the same time.
I guess this example also shows that it might be desirable to have independent versions of the app and the package. If the package does not change at all... why bump its version?
So maybe the version in app/info.xml
could be used when referring to the version of the app and package.json
would hold the version of the package.
What are we using the version in package.json for right now?
I think this could be beneficial for other apps too. Just as a thought experiment... what if viewer provided a package that allowed
import { registerHandler } from '@nextcloud/viewer'
It could still use
OCA.Viewer
underneath. But it would easily allow apps using this api to confirm that the package actually exports theregisterHandler
function.
The first issue I see with that is... versionning.
Doing so means I suddenly have to manage compatibility between apps. This complicates maintenance profoundly.
You could import registerHandler
from the latest @nextcloud/viewer
but run this on an app that is compatible with nextcloud 22 only (while latest @nextcloud/viewer
would be compatible with nextcloud 24 only)
How would we manage that then? I honestly don't know :see_no_evil:
The first issue I see with that is... versionning. Doing so means I suddenly have to manage compatibility between apps. This complicates maintenance profoundly. You could import
registerHandler
from the latest@nextcloud/viewer
but run this on an app that is compatible with nextcloud 22 only (while latest@nextcloud/viewer
would be compatible with nextcloud 24 only)
With the package you can have a chance to manage compatibility. With globals like OCA.Viewer
you don't. There's nothing stopping users from using `OCA.Viewer.open({path: ...}) in an app that is compatible with a Nextcloud that does not include the Viewer app.
Since packages can be used to manage dependencies and compatibilities there may be the implicit assumption that we do so - so maybe we should.
How would we manage that then? I honestly don't know :see_no_evil:
First of all... Let's not manage compatibility between arbitrary Nextcloud apps. My understanding is that apps should not depend on other apps and we're sticking to that for the most part... except for a few apps that are shipped with Nextcloud.
So we're talking about managing compatibility of other apps with the shipped apps and therefore mostly with the underlying Nextcloud version. For text we're planning to use major versions that map the Nextcloud release. So first package would be @nextcloud/text
version 25.x.y.
So far we've been extending the functionality of text
and that's what we expect to do moving forward. So @nextcloud/text
Versin 25.0.0 will provide a way to render markdown created by text version 25 and earlier.
I think this way around the version prevents a pretty good indicator. If you want your app to work with Nextcloud 26... you'll need version 26 of @nextcloud/text
. But i guess that's also the easy side of things.
The viewer packages dependency on at least say Nextcloud 23 is harder to express. We could represent it as a peer depenency. @nextcloud/viewer
could peer depend on @nextcloud/version
23.0.0 or later. However now it would seem that if i install @nextcloud/viewer
and it autoinstalls the peer dependency as @nextcloud/version
25.0.0 my app will require Nextcloud 25.
What we really want to express is a range. @nextcloud/viewer
could peer depend on @nextcloud/minVersion
23 and @nextcloud/maxVersion
25 - expressing that the API provided can be used with Nextcloud 23 - 25. The minVersion
and maxVersion
package could implement a checkVersion
function that reads the minVersion
/ maxVersion
from the appInfo and makes sure it does not exceed the range specified by the packages.
In the app that uses @nextcloud/viewer
and is compatible with Nextcloud 22 only the minVersion
packages checkVersion
function would indicate a mismatch. One way of integrating this would be to make webpack-vue-config
peer depend on minVersion
and maxVersion
and call their checkVersion
functions before running the webpack build process.
Of course one can still work around that by not using webpack-vue-config
. But i guess the goal here is to make sure we do not hand people a footgun rather than enforcing compliance.
What we really want to express is a range.
@nextcloud/viewer
could peer depend on@nextcloud/minVersion
23 and@nextcloud/maxVersion
25 - expressing that the API provided can be used with Nextcloud 23 - 25. TheminVersion
andmaxVersion
package could implement acheckVersion
function that reads theminVersion
/maxVersion
from the appInfo and makes sure it does not exceed the range specified by the packages.
We could actually just have @nextcloud/version
and specify range with npm
"peerDependencies": {
"@nextcloud/version": ">=22 <=25"
}
I really like the idea :rocket: I'm more and more in favour of having those apps exporting their methods.
With just @nextcloud/version
as you described it... Would you test the apps minVersion
and maxVersion
as specified in appinfo are okay with its dependencies or would you leave that to the developer to check?
With just
@nextcloud/version
as you described it... Would you test the appsminVersion
andmaxVersion
as specified in appinfo are okay with its dependencies or would you leave that to the developer to check?
Complicated to enforce, as always. We cuold draft a best practice to states we should test all supported versions imho. But lots of various repos test the min only, or master only. There is not much consistency I think for now :)
So, after a bit of thoughts and investigation. Seeing the issues with https://github.com/nextcloud/text/pull/2614, I think it's a bad idea to have them both in the same root. A package.json is representing a single element. Having both the app AND the package is actually going to mess with the dependencies and make things messy in my opinion.
What I suggest we do:
MyApp/
├─ node_modules/
├─ dist/
│ ├─ myapp.cjs
│ └─ myapp.js
├─ js/
│ └─ myapp-main.js
├─ src/
│ ├─ package/
│ │ ├─ node_modules/
│ │ ├─ lib/
│ │ │ └─ index.js
│ │ └─ package.json
│ └─ main.js
├─ package.json
│
This decision have been taken after the following discussion: https://github.com/nextcloud/standards/issues/3
Sometimes it may be useful to provide an npm package to ease the integration with an existing app or to provide reusable code.
For example text markdown rendering is reused in collectives and might be useful to other apps.
Splitting the text app into a package and the app would create extra overhead in particular for backporting fixes - but also in terms of testing, issue tracking etc. - we'd probably end up with a
nextcloud/text
repo and anextcloud/nextcloud-text
repo. Sounds confusing and might be asking for trouble.So instead we'd like to build the
@nextcloud/text
npm package from the source code of the text app. This adds a new build target. In our case it also adds some tooling because we want to build esm packages - but it's far less intrusive than splitting the app in two repos.The
package.json
file so far does not contain the fields relevant to packaging ('files', 'main', 'module', 'type') etc. We added them and we have a working prototype here: https://github.com/nextcloud/text/pull/2386 So far it builds the package nicely and we can use it in collectives.There's a bunch of metadata that is used both by the package and the app:
name
inpackage.json
- Version number in
package.json
dependencies
inpackage.json
Readme.md
Seeing the issues with https://github.com/nextcloud/text/pull/2614, it's a bad idea to have them both in the same root.
We took the decision to not share package.json
MyApp/
├─ node_modules/
├─ dist/
│ ├─ myapp.esm.js
│ ├─ myapp.cjs.js
│ └─ myapp.js
├─ js/
│ └─ myapp-main.js
├─ src/
│ ├─ package/
│ │ ├─ node_modules/
│ │ ├─ lib/
│ │ │ └─ index.js
│ │ └─ package.json
│ └─ main.js
├─ package.json
│
@max-nextcloud please confirm you're ok with this closing template (above) and then we can sloe and create a final issue summarising the decision taken here ?
Makes sense from my perspective 👍
Sometimes it may be useful to provide an npm package to ease the integration with an existing app or to provide reusable code.
For example text markdown rendering is reused in collectives and might be useful to other apps.
Splitting the text app into a package and the app would create extra overhead in particular for backporting fixes - but also in terms of testing, issue tracking etc. - we'd probably end up with a
nextcloud/text
repo and anextcloud/nextcloud-text
repo. Sounds confusing and might be asking for trouble.So instead we'd like to build the
@nextcloud/text
npm package from the source code of the text app. This adds a new build target. In our case it also adds some tooling because we want to build esm packages - but it's far less intrusive than splitting the app in two repos.The
package.json
file so far does not contain the fields relevant to packaging ('files', 'main', 'module', 'type') etc. We added them and we have a working prototype here: https://github.com/nextcloud/text/pull/2386 So far it builds the package nicely and we can use it in collectives.There's a bunch of metadata that is used both by the package and the app:
name
inpackage.json
package.json
dependencies
inpackage.json
Readme.md
I'll go through them one by one:
Name
The package is currently called
@nextcloud/text
. https://github.com/nextcloud/webpack-vue-config/pull/338 proposes a change to the webpack-vue-config to remove the@nextcloud
prefix from the app name when building the js assets. This way naming the package that goes along withAPP
@nextcloud/APP
would work nicely.Version number
Once the package is released i do not see any reason to not follow the version numbering of the app. We're currently using
0.x.0
numbers to indicate this is work in progress.Dependencies
The package only covers parts of text functionality. Therefor it only needs some of texts dependencies. We turned all other dependencies into
devDependencies
. This prevents additional dependencies for the package and still works nicely for building the app.Readme.md
The Readme is used to present the app on apps.nextcloud.com and as part of a package it's show on npm: https://www.npmjs.com/package/@nextcloud/text
From my point of view this is the strongest conflict in reusing metadata between the app and the package. The audience for an app is different from the one of the package.
However i think this could be solved by adding a small section on how to use the package. Knowing what the app is about and how development works is relevant for both the app and the package anyway.