-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NPM packages for apps #3
Comments
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 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 What are we using the version in package.json for right now? |
The first issue I see with that is... versionning. How would we manage that then? I honestly don't know 🙈 |
With the package you can have a chance to manage compatibility. With globals like Since packages can be used to manage dependencies and compatibilities there may be the implicit assumption that we do so - so maybe we should.
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 So far we've been extending the functionality of 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 The viewer packages dependency on at least say Nextcloud 23 is harder to express. We could represent it as a peer depenency. What we really want to express is a range. In the app that uses Of course one can still work around that by not using |
We could actually just have "peerDependencies": {
"@nextcloud/version": ">=22 <=25"
} I really like the idea 🚀 |
With just |
Complicated to enforce, as always. We cuold draft a best practice to states we should test all supported versions imho. |
So, after a bit of thoughts and investigation. Seeing the issues with nextcloud/text#2614, I think it's a bad idea to have them both in the same root. What I suggest we do:
|
Single repository for app and related npm packageThis decision have been taken after the following discussion: #3 Initial idea
DrawbacksSeeing the issues with nextcloud/text#2614, it's a bad idea to have them both in the same root.
We took the decision to not share package.json Recommendations:
|
@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: nextcloud/text#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
. nextcloud-libraries/webpack-vue-config#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.
The text was updated successfully, but these errors were encountered: