Skip to content
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

#194 Switch to peer dependencies for @theia dependencies #135

Merged
merged 2 commits into from
Jan 20, 2023
Merged

Conversation

ndoschek
Copy link
Contributor

Part of eclipse-emfcloud/emfcloud#194

Contributed on behalf of STMicroelectronics

Part of eclipse-emfcloud/emfcloud#194

Contributed on behalf of STMicroelectronics
@tortmayr tortmayr requested review from martin-fleck-at and tortmayr and removed request for martin-fleck-at January 18, 2023 15:00
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nina, I'm looking forward to using peer Dependencies for core Theia extensions. This should help in resolving the dependency hell in some of our downstream projects.
However, I noticed a couple of issues:

The example packages still reference Theia extensions as dependencies instead of peerDependencies. We should fix this to ensure that the browser-app is the single source of truth and has the sole responsibility of providing the actual used Theia versions.

Also, to fully support peerDependencies in donwstream projects we need a fix that is only available with Theia >=1.33. (eclipse-theia/theia@2ec7d87). Without this fix the module loading order of Theia extensions that rely on peer dependencies might get mixed up and break the application in certain cases.

We need to update the minimum Theia verison requirements anyways as I'm fairly certain that we are not compatible with Theia version 1.0.0 since quite a long time 😉
I would suggest to directly update the minimum Version to 1.33.0

- Update to Theia 1.33.0
- Switch to peer dependencies also for example packages

Part of eclipse-emfcloud/emfcloud#194

Contributed on behalf of STMicroelectronics
@ndoschek ndoschek requested review from tortmayr and eneufeld January 19, 2023 15:29
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fast update. LGTM 👍🏼

One last thing, not sure if it's an issue but if I execute yarn theia check:dependencies I get two errors regarding the ws package:

❯ yarn theia check:dependencies
yarn run v1.22.19
$ /home/tobias/Git/OpenSource/emfcloud/emfcloud-modelserver-theia/node_modules/.bin/theia check:dependencies
✅ Found 7 workspaces
✅ Found 1285 dependencies
🟠 Found 2 issues

#1  ws in @eclipse-emfcloud/modelserver-theia [not-hoisted]
    error Dependency ws is not hoisted.
    8.5.0 in packages/modelserver-theia/node_modules/ws/package.json
    7.5.9 in node_modules/ws/package.json

#2  ws in @eclipse-emfcloud/modelserver-theia [multiple-versions]
    error Multiple versions of dependency ws found.
    8.5.0 in packages/modelserver-theia/node_modules/ws/package.json
    7.5.9 in node_modules/ws/package.json


ℹ️  Use yarn why <package-name> to find out why those multiple versions of a package are pulled.
ℹ️  Try to resolve those issues by finding package versions along the dependency chain that depend on compatible versions.
ℹ️  Use resolutions in your root package.json to force specific versions as a last resort.

@tortmayr
Copy link
Contributor

And on a side note, in GLSP we maintain a Theia version compability matrix in the README. Maybe we do the same here.

@ndoschek
Copy link
Contributor Author

Thanks @tortmayr, will keep that in mind, regarding the compat section, that's a good point, we will add that with #76

@ndoschek ndoschek mentioned this pull request Jan 20, 2023
@ndoschek ndoschek merged commit e8752f2 into master Jan 20, 2023
@ndoschek ndoschek deleted the issues/194 branch January 20, 2023 10:48
ndoschek added a commit that referenced this pull request Jan 20, 2023
- Follow-up of #135: Align duplicate `ws` dependency version
- Update eslint config to use dynamic year in headers
- Re-generate yarn.lock to update several dependencies and resolve several dependabot alerts
- Due to CI problems after Theia update: Increase pod resource limits
ndoschek added a commit that referenced this pull request Jan 20, 2023
- Follow-up of #135: Align duplicate `ws` dependency version
- Update eslint config to use dynamic year in headers
- Re-generate yarn.lock to update several dependencies and resolve several dependabot alerts
- Due to CI problems after Theia update: Increase pod resource limits
ndoschek added a commit that referenced this pull request Jan 20, 2023
- Follow-up of #135: Align duplicate `ws` dependency version
- Update eslint config to use dynamic year in headers
- Re-generate yarn.lock to update several dependencies and resolve several dependabot alerts
- Remove obsolete custom isAxiosError check and reuse built-in type check
- Due to CI problems after Theia update: Increase pod resource limits
ndoschek added a commit that referenced this pull request Jan 25, 2023
- Follow-up of #135: Align duplicate `ws` dependency version
- Update eslint config to use dynamic year in headers
- Re-generate yarn.lock to update several dependencies and resolve several dependabot alerts
- Remove obsolete custom isAxiosError check and reuse built-in type check
- Due to CI problems after Theia update: Increase pod resource limits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants