fix '@theia' implicit dependencies #5791
Conversation
vince-fugnitto
added
quality
|
@marcdumais-work what's unfortunate is that |
Fixes #5790 - fixed the `@theia/` `no-implicit-dependencies` warnings from extensions which used dependencies directly without declaring them in their package.json - fixed issue in `@theia/plugin-ext` regarding incorrect import paths Signed-off-by: Vincent Fugnitto <[email protected]>
vince-fugnitto
force-pushed
the
vf/GH-5790
branch
from
8d081ba
to
51a443e
Compare
vince-fugnitto
changed the title
as an explicitly to @theia/plugin-ext`
|
We will need to move class somewhere else then, let's add it for now. It cannot work otherwise. |
| @@ -18,9 +18,8 @@ import URI from 'vscode-uri/lib/umd'; | |||
| import * as theia from '@theia/plugin'; | |||
| import * as Converter from '../type-converters'; | |||
| import * as model from '../../api/model'; | |||
| import { DocumentsExtImpl } from '@theia/plugin-ext/src/plugin/documents'; | |||
There was a problem hiding this comment.
that's a good catch, how did you find it?
There was a problem hiding this comment.
tslint was complaining that @theia/plugin-ext was implicitly depending on itself so I
checked as to how / why 🙈
There was a problem hiding this comment.
Was it a warning as well, it would be good to have it as error.
There was a problem hiding this comment.
It was a warning, I guess we need a custom rule to find out if an extension is trying to import istelf this way.
|
@benoitf are you fine with me merging? |
|
@vince-fugnitto @marcdumais-work We don't have code ownership. Everybody should be able to change, review and discuss whatever comes. We've added @evidolob @benoitf because they wanted to be notified about changes in the plugin system. It does not mean that you need an approve. One approve from anybody who verified design and tested changes is fine to merge. You also don't need approve from me on PRs to core. I will comment if i see something suspicious. Let's remove code owners file if it is confusing. I see already several PRs where it happens and no response. We could have a rule instead that any nontrivial PR should sit at least 2 days after being approved. So everybody for sure had time to have a look. Also if it breaks we revert it within 2 days. Should we discuss it on next dev meeting? |
Sounds good, inititally I thought it meant that were required to review changes from the plugin system (based on past PRs) but if its meant to only notify them of changes then I'll keep that in mind for the future :)
Yes I understand. Generally I feel more comfortable adding you to reviews that are significant or may require more in depth design decisions since you have valuable insight. I'd rather wait for a PR to be complete and implemented well before I merge.
I think it's fine, I know they wanted to be notified of changes to the plugin system but with the newer ways of working I understand that no one has code ownership anymore.
If you'd like to bring it up I'd be fine with that. |
What it does
Fixes #5790
@theia/no-implicit-dependencieswarnings from extensionswhich used dependencies directly without declaring them in their package.json
@theia/plugin-extregarding incorrect import pathsHow to test
@theia/plugin-extor@theia/plugin-ext-vscodeexcluding@theia/mini-browseras a dependency. (ensure that@theia/mini-browseris not being pulled by another dependency as well.Review checklist
Reminder for reviewers
Signed-off-by: Vincent Fugnitto [email protected]