re-export common packages

[paul-marechal]

What it does

The goal of this refactoring is to make downstream application builds more stable by removing implicit dependencies and exporting common packages from @theia/core/shared/<package>.

This method should not break dependents. See commit message for explanation.

Fixes #8403

Most changes are just about updating all locations where implicit dependencies are used. I think this is a good change for the framework as it is an opt-in feature, doesn't break dependents, and improves the build stability for application developers making use of it.

How to test

This is a refactoring: Everything should work like before.

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

[paul-marechal]

Ok so the last step is to find a way to configure webpack to ignore non-browser stuff from @theia/application-package and it should work just like before. I might break the environment file apart in its own package as the simpliest solution.

[paul-marechal]

Fixed CI and the frontend build by re-exporting @theia/application-package/lib/environment as @theia/core/shared/application-package/lib/environment. I did not need to create a new package nor edit webpack configs.

[kittaakos]

Related: #7303

[paul-marechal]

Added links to the respective npm page for each re-exported package in EXPORTS.md.

[paul-marechal]

Updated packages/core/README.md to list the re-exported packages instead of being in its own file. It makes discovering the mechanism easier. The readme is now autogenerated based on the contents of README.in.md.

[vince-fugnitto]

I confirmed the following:

The no-extraneous-dependencies rule works well (fixes #7079 if you'd like to automatically close):

error '@theia/debug' should be listed in the project's dependencies. Run 'npm i -S @theia/debug' to add it import/no-extraneous-dependencies

The new eslint plugin properly picks up violations of not using the shared dependencies re-exported by @theia/core (attempting to use inversify the old way):

"inversify" is a @theia/core shared dependency, please use "@theia/core/shared/inversify" instead. eslint(@theia/shared-dependencies)

[paul-marechal]

@tsmaeder will this change cause issues with how you consume Theia?

[tsmaeder]

@tsmaeder will this change cause issues with how you consume Theia?

As it's an optional change, I'm not sure it's the best use of our effort.

[paul-marechal]

@tsmaeder does that mean that this change is fine to go as is, or are you against merging?

edit:

As it's an optional change, I'm not sure it's the best use of our effort.

This change goes in the way of improving the framework's stability for people consuming it through NPM, so I'd argue that it is not simply "optional". There's always the option to refuse changes in general, so that would make this change optional in that aspect? But this patch fixes a class of issues, it's not like I just want to make things different.

Regarding the effort it's a do once and forget about it. Search and replace, follow the eslint errors and done. People writing Theia extensions or Theia applications by pulling packages from NPM will not even be affected. Only forks would need to align.

[tsmaeder]

One think I still don't understand is if types will be compatible when I import them directly instead of via reexports: for example, if I extend a class and override a method, will the parameter types be compatible or do I need to change my source? In brief: is the change potentially breaking?

[paul-marechal]

This change is not breaking in that regard, it makes it even more reliable: You will always use the same types than those used by @theia/core. If you happened to use a different version than what @theia/core uses and types happen to be incompatible then you will need to update your code indeed. But this is what this PR fixes, by aligning dependents with what is used by the framework.

edit: This alignment is currently only enforced in our repo and potentially forks, downstream extensions are encouraged to do the same but it wouldn't be enforced there.

[tsmaeder]

@marechal-p duck typing works for interfaces, but what if a return type is a class, for example? They may be structurally the same, even refer to the same version, but does it compile?

[paul-marechal]

but what if a return type is a class, for example?

@tsmaeder Then it is important to understand how one would end up with two distinct references to what looks like the same class.

This usually happens when the same package is installed twice under different versions e.g. doing yarn why <package> returns multiple entries. This can be due to unlucky hoisting due to any dependency from the whole project that depends on the same package than @theia/core but with an incompatible range: Yarn will install both versions to satisfy everyone, and it might pull the rug under your feet and make your implicit dependency require a different version than which from @theia/core.

The same package installation mismatch scenario can happen if you don't rely on implicit dependencies but rather try to explicitly depend on the same package as @theia/core. In this case if you happen to lag behind while @theia/core updates its own dependency and you don't, you might end up with two installations for the same package again.

In both scenarios, you have two packages. If both export a class and they are structurally the same, since they come from different packages they will be two different references and type compatibility will only depend on whether or not those classes declare private fields: if they do declare private fields then TS will complain at build time when you use one in place of the other. It does not matter that the private fields are identical, the class definitions are separate so TS complains. See error and success.

Using re-exports prevents this completely since the types involved will always be the same and hoisting will never cause these scenarios to happen. Dependents will only need to update code if @theia/core updates one of its own shared dependencies to a new incompatible version compared to the one before. But this was already the case, implicitly.

Note that type incompatibility issue will never happen if your code requires the classes from the same package version, as it will most likely mean that the package is not duplicated due to mismatched ranges.

This is a complex issue due to hoisting. This change gets rid of our dependence on hoisting.

[paul-marechal]

@tsmaeder I got burned when doing one of the earliest yarn upgrade PR: #6255

I had to fight with how types were imported/exported, and implicit dependencies didn't help. The gist to fixing the issue was to re-export lsp types from a central Theia file and use that as source of truth everywhere else.

[tsmaeder]

But if I do this in @theia/pkg:

import  { Foo } from '@theia/shared/pkg'

export function foo(): Foo {
  return new Foo();
}

and I currently have code that does

import { Foo } from 'pkg'
import { foo } from '@theia/pkg'

var myFoo: Foo = foo();

I would expect the two Foo types to be not assignment compatible (if they have private vars). What I'm trying to understand is this: can I get compilation errors in existing code with this change. I think I can.

[paul-marechal]

@tsmaeder the issue you are describing here can already happen without my changes, even if both locations just import pkg. It depends on the hoisting layout. As long as require pulls things from different packages you will have the issue you are mentioning. But in your example it may or may not break, depending if import { Foo } from 'pkg' resolves to the same package that @theia/core/shared/pkg points to or not. Same issue we currently have without this change.

My patch allows us to get rid of that issue by allowing everyone to always refer to the same package thanks to re-exporting things. Here is an example showing how re-exporting is preserving types: ts-class-re-export.zip. Note how the recopied class fails, but the re-exported type works.

edit: But you are right that it can still break if people keep using implicit dependencies, but only if there's hoisting issues. Just like is currently happening, this change doesn't make this case worse or better. It only improves things when @theia/core/shared/<package> is used.

[paul-marechal]

@tsmaeder here's another way to put the benefits of this PR:

This change prevents the issue you are talking about in this repository, and enforces it. After merging, if dependents keep using implicit dependencies then the issue you are mentioning can still happen, but it will be caused by their own implementations and not by the core packages anymore. Extenders that wish to also prevent the issue can now use this @theia/core/shared/ mechanism. Without this change not much can be done except tippy-toeing with those dependencies like we do today.

[paul-marechal]

I'll resolve conflicts before merging somewhere around Thursday/Friday of this week.

[kittaakos]

@marechal-p, this PR won't fix this problem, right?

If we merge the PR, I can still create a Theia extension that has a couple of dependencies that depend on fs-extra. For example: fs-extra@8. It has a different version than we use in Theia: fs-extra@4. If there are more dependencies that depend on fs-extra@8 than Theia extensions that depend on fs-extra@4, fs-extra@8 will be hoisted, and re-exporting common dependencies has no effect. Can you please help with this? I might be overlooking something. Thank you!

(If my example needs more clarification, let me know.)

[paul-marechal]

@kittaakos as long as the Theia extension requires fs-extra through @theia/core/shared/fs-extra then even when fs-extra is duplicated it will keep working, so it should fix the problem you linked.

See example: fs-extra-duplicated.zip

Package a depends on fs-extra@9 and package b on fs-extra@6. Yarn will always make sure that each package sees what it explicitly requested. And as you can see, when a re-exports it's own fs-extra for b, the correct fs-extra@9 module is passed around. This is the exact same mechanism I used in this PR.

This means that hoisting will keep happening, but it won't de-stabilize builds since packages will be able to precisely import the same shared packages from the places that explicitly depend on them.

[kittaakos]

See example: fs-extra-duplicated.zip

👍 Thanks for the example.

so it should fix the problem you linked.

That would be great.

[paul-marechal]

I'll keep an eye out for any breakage but I don't expect any. I'll also assist with PRs if people need help rebasing.