repo: fix implicit dependencies

[paul-marechal]

What it does

Not explicitly declaring dependencies is being a really bad NPM citizen.

Static tools like `Webpack` might be able to infer dependencies based on
actual imports done from the scripts, but other tools such as
`electron-builder` will fail since they seem to analyze the manifest
files.

This commit does 2 things:

- Add a new script that will check for duplicate dependencies and ensure
that all packages here are using the same ranges. This should minimize
typing issues.
- Add explicit declaration of dependencies, as enforced by the
no-extraenous-dependencies from eslint.

Fixes #7079

How to test

Build and tests should pass, no regressions.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

I verified the changes by removing a dependency from an ext package.json and confirming that it fails the build. 👍 Please rebase the changes to pick up both #7119, and #6933 (there are extensions present which are deprecated and have been removed).

[akosyakov]

theia cli tool has a command to check it, we should use it instead:

theia/dev-packages/cli/src/theia.ts

Lines 125 to 144 in d5a60bb

	.command({
	command: 'check:hoisted',
	describe: 'check that all dependencies are hoisted',
	builder: {
	'suppress': {
	alias: 's',
	describe: 'suppress exiting with failure code',
	boolean: true,
	default: false
	}
	},
	handler: args => {
	try {
	checkHoisted(args);
	} catch (err) {
	console.error(err);
	process.exit(1);
	}
	}
	})

It is important, since downstream electron projects should be able to run the same check.

[paul-marechal]

The thing is, electron-builder seems to create its own dependency layout when bundling, which would be different from what yarn does.

Example: yarn was correctly hoisting fs-extra, but when bundled it was not. This made the app fail at runtime. The solution was to have whatever extension depending on fs-extra to explicitly declare so, then tooling was able to correctly install things.

My conclusion is that we should not rely on hoisting.

[akosyakov]

It is not about electron-builder only. Someone develops a product and wants to check whether there is not duplicate dependencies for custom and core extensions then he should run exactly the same script which we run to check duplicate dependencies in this repo. check:hoisted is the script

cc @kittaakos

[paul-marechal]

If we want singleton packages, then we would have to deal with peer dependencies

https://classic.yarnpkg.com/blog/2018/04/18/dependencies-done-right/

I might open an issue later to discuss that more in detail, I really think that our current system is too fragile.

[akosyakov]

I don't really care about peer dependencies. The point is that we don't need 2 scripts doing the same things. Please use check:hoisted. If everything is hoisted it means that there are no duplicates.

[RomanNikitenko]

It's strange, but I have some problems related to task schema support for this branch.

Actual:

Expected:

The same problem for settings.json file.
Could someone check it.

thanks in advance!

[paul-marechal]

@RomanNikitenko I think it should be fixed now.

Turns out we don't need the resolutions field anymore? Tell me if I missed anything.

[RomanNikitenko]

Tested after update: the bug described above has been fixed

[akosyakov]

Why do we do exact match and don't use caret ranges as we do it usually?

[paul-marechal]

PhosphorJS is broken up in several bits, and we used to only import one bit and implicitly import the others. If we want to make everything explicit, I was looking for an easy way of declaring it. Just putting 1 means "any version with the major number being 1". This always targets the latest version for major 1, so it is equivalent to ^1.0.0. I don't see the use of being more precise about it, like ^1.2.3, since we mostly want the latest anyway.

[paul-marechal]

If PhosphorJS releases a version 2.0.0, Theia will most likely not be compatible without us updating the API used. This to me essentially mean that we are compatible with v1 in a general manner, hence why it is easier to just say 1.

[akosyakov]

There won't Phosphor.JS 2. It was deprecated and we going to copy relevant bits in @theia/core eventually and inverisfy them.

[akosyakov]

I am not sure about it. It does not seem to solve any real issue, but cause a lot of boilerplate and requires scripts to support such boilerplate. It would be also ill advised to redeclare dependencies to inversify.js and phosphor for down stream projects, they have to rely on versions coming with @theia/core always. Otherwise we will break them whenever we bump up versions. It is not acceptable for us, especially after v1.

I think we should rather invest time to create custom linting rules which does not require reduplicating dependencies and can be reused by downstream projects. We have a need for rules not only to avoid duplicate depenencies, see #5873 and #1238

[paul-marechal]

This PR is supposed to fix shady issues like reported in Spectrum: https://spectrum.chat/theia/general/our-theia-electron-builder-app-no-longer-starts~f5cf09a0-6d88-448b-8818-24ad0ec2ee7c?m=MTU3MDcxNjg1OTMwNA==

If we keep things implicit, we'll keep having to dance around whatever yarn is doing. Explicitly entering dependencies removes this kind of issues, since we tell yarn who needs what in order to always meet dependency requirements. Less headaches this way IMO.