Remove unnecessary dependencies

[skjnldsv]

They are not needed as they are automatically handled by npm7 and the associated configs packages

Closes #2612
cc @susnux

Btw, you seems to have a lot of dependencies in dev that should be in the dependencies section ? Please double-check, they might be here because of the secondary vite building package, 🤷

[susnux]

For the app package it should make not difference if you put the dependencies into dev- or dependencies. But for the @nextcloud/text package that would make a difference.

[skjnldsv]

For the app package it should make not difference if you put the dependencies into dev- or dependencies. But for the @nextcloud/text package that would make a difference.

Well, I'd say since they are runtime deps, they should be in dependencies, since we're exclusing those anyway from the final bundles:

text/vite.config.js

Line 18 in 96f0cdd

external: Object.keys(dependencies),

[skjnldsv]

Well, according to the components used in the @nextcloud/text package, I see absolutely no usage of those lib. They should be in dependencies....

text/src/components/RichTextReader.vue

Lines 28 to 30 in 96f0cdd

	import BaseReader from './BaseReader.vue'
	import RichText from './../extensions/RichText.js'
	import markdownit from './../markdownit/index.js'

text/src/extensions/RichText.js

Lines 23 to 46 in 96f0cdd

	import { Extension } from '@tiptap/core'
	/* eslint-disable import/no-named-as-default */
	import Document from '@tiptap/extension-document'
	import Paragraph from '@tiptap/extension-paragraph'
	import Text from '@tiptap/extension-text'
	import Blockquote from '@tiptap/extension-blockquote'
	import OrderedList from '@tiptap/extension-ordered-list'
	import ListItem from '@tiptap/extension-list-item'
	import Code from '@tiptap/extension-code'
	import CodeBlock from '@tiptap/extension-code-block'
	import HorizontalRule from '@tiptap/extension-horizontal-rule'
	import Dropcursor from '@tiptap/extension-dropcursor'
	import HardBreak from './HardBreak.js'
	import Table from './../nodes/Table.js'
	import Image from './../nodes/Image.js'
	import Heading from './../nodes/Heading.js'
	import BulletList from './../nodes/BulletList.js'
	import TaskList from './../nodes/TaskList.js'
	import TaskItem from './../nodes/TaskItem.js'
	import Callout from './../nodes/Callouts.js'
	/* eslint-enable import/no-named-as-default */
	import { Strong, Italic, Strike, Link, Underline } from './../marks/index.js'

text/src/components/BaseReader.vue

Lines 28 to 29 in 96f0cdd

	import { Editor } from '@tiptap/core'
	import { EditorContent } from '@tiptap/vue-2'

[vinicius73]

Can we just remove it?
It not affects the host's npm.

[skjnldsv]

No, this is what we requires at Nextcloud.
Min npm7 and node14. We move alltogether when npm7 got out and communicated about it already

[max-nextcloud]

Well, according to the components used in the @nextcloud/text package, I see absolutely no usage of those lib. They should be in dependencies....

My understanding is that if they are not used at all vite will not package them. But if they are in dependencies apps that use @nextcloud/text will start packaging them, right? Is that what we want? For example should collectives start packaging them?

[max-nextcloud]

This is a peer dependency of @nextcloud/eslint. My understanding is that peerDependencies of dependencies should be added as devDependencies to ensure they are installed and keep track of their versions even though npm will auto install peer dependencies nowadays.

[susnux]

that is not a good idea, as that might lead to version conflicts, while npm will ensure the correct version is installed

[skjnldsv]

This is a peer dependency of @nextcloud/eslint. My understanding is that peerDependencies of dependencies should be added as devDependencies to ensure they are installed and keep track of their versions even though npm will auto install peer dependencies nowadays.

It's the other way, we made sure each config dependency handle their required versions themselves.
All the other apps are doing it that way, it actually helped us a lot to reduce noise and ensure further compatibilities

[max-nextcloud]

So far my thinking was this:

1. if it's a runtime dependency of the package -> add to dependencies
2. if it's not a runtime dependency of the package but a dependency for building the app -> add to devDependencies
3. if it's a peerDependency of one of our dependencies or devDependencies -> add to devDependencies.

The goal of 1. and 2. is to depend only on the things the package actually requires. If we forget something here it will be bundled with the package and the package will still work - but it will grow in size. I was thinking about adding a CI check for that as it would be hard to notice otherwise.

About 3. I was mostly following what i heard from @vinicius73 and thinking this makes conflicts explicit and defines a working set of dependencies. I'm fine with fully relying on npm magic here. I think we should pick one strategy and stick to it - maybe even document it and standardize it?

[vinicius73]

About 3. I was mostly following what i heard from @vinicius73 and thinking this makes conflicts explicit and defines a working set of dependencies. I'm fine with fully relying on npm magic here. I think we should pick one strategy and stick to it - maybe even document it and standardize it?

We have alt least 2 problems.

• NPM and Node inconsistency: Node v14 is our development target, but we use NPM v7, a legacy NPM version who is not bundled with Node.

• We are using the same package.json to be used in an application and a package.

From the perspective of an application, every dependency matters. Those dependencies can be declared as dependencies (who will be bundled in production) or devDependencies (who will not be bundled in production).
From the perspective of a package, we must avoid bundle dependencies with the release package.

We can choose between dependencies, devDependencies and peerDependencies, but peerDependencies only makes sense in a package, not in an application.

[susnux]

3. if it's a peerDependency of one of our dependencies or devDependencies -> add to devDependencies.

...

About 3. I was mostly following what i heard from @vinicius73 and thinking this makes conflicts explicit and defines a working set of dependencies. I'm fine with fully relying on npm magic here. I think we should pick one strategy and stick to it - maybe even document it and standardize it?

3. was the way to go with npm 3-6, but led to version conflicts as you have to manually check that the peer dependencies and your dev dependencies match.
   And monitor any bot output as a dependency update might break the manually pinned peer dependencies

• NPM and Node inconsistency: Node v14 is our development target, but we use NPM v7, a legacy NPM version who is not bundled with Node.

Node 14 bundles npm 6, I guess 7 is used because of it's huge improvements(?). In 10 months node 14 will be EOL and the current LTS is node 16 bundling npm 8.
So there should be no problem with that versions.

[skjnldsv]

About 3. I was mostly following what i heard from @vinicius73 and thinking this makes conflicts explicit and defines a working set of dependencies. I'm fine with fully relying on npm magic here. I think we should pick one strategy and stick to it - maybe even document it and standardize it?

This is already standardized at Nextcloud with those global packages.We did a lot of work over the last year and a half to go that direction.
With npm 7 came the fix for peer dependencies. We took advantage of that and migrated to global packages.

[skjnldsv]

• NPM and Node inconsistency: Node v14 is our development target, but we use NPM v7, a legacy NPM version who is not bundled with Node.

No, this is not an issue

text/package.json

Lines 91 to 94 in 96f0cdd

	"engines": {
	"node": "^14.0.0",
	"npm": "^7.0.0"
	},

You just have to be compliant about what your app requires

[max-nextcloud]

Okay... so removing the peer dependencies of our dependencies for package.json seems to be the agreed upon way to go within Nextcloud. I'm fine with that. And that will fix the issue this PR is addressing, right?

I think dependencies that are not runtime dependencies of the package should stay in devDependencies and not become dependencies. There's a number of them where it's hard to make a clear decision. I moved everything into devDependencies that is not imported by the package - as that was an easy criterion to apply. But one could obviously argue that a package that exposes a vue component depends on vue.

I'm okay with moving on with this PR but i'd probably move for example @nextcloud/initial-state back into dev dependencies as there's no reason for an app that uses @nextcloud/text to also include @nextcloud/initial-state if it's not using it.

[skjnldsv]

I'm okay with moving on with this PR but i'd probably move for example @nextcloud/initial-state back into dev dependencies as there's no reason for an app that uses @nextcloud/text to also include @nextcloud/initial-state if it's not using it.

The dependency might be installed yes, but it will not be part of the final bundle of the app then
In any case, for me this is an extra point against nextcloud/standards#3. If you want to do that, please use a differet package.json. Having unrelated depdndencies seems like a bad idea :/