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

removed debug-nodejs and java-debug #6113

Closed
wants to merge 1 commit into from
Closed

removed debug-nodejs and java-debug #6113

wants to merge 1 commit into from

Conversation

svenefftinge
Copy link
Contributor

This PR completely removes the two native debug adapter adapter, as the vs code extensions can be used instead.

Fixes #6111

@akosyakov
Copy link
Member

@svenefftinge changes to yarn.lock are missing:

Did you update and commit your 'yarn.lock' ?

@akosyakov akosyakov added debug issues that related to debug functionality vscode issues related to VSCode compatibility labels Sep 5, 2019
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I'm good with removing, but please update yarn.lock

@kittaakos
Copy link
Contributor

please update yarn.lock

And the .travis.yml.

From the CI:

Changes not staged for commit:
1531  (use "git add <file>..." to update what will be committed)
1532  (use "git checkout -- <file>..." to discard changes in working directory)
1533
1534	modified:   .travis.yml
1535	modified:   yarn.lock

@marcdumais-work
Copy link
Contributor

So this PR resolves the failing CI. But it also removes the debugging feature for node and Java. Should things be left like this for the example applications or should the corresponding vscode extensions be added, fetched from an alternate place?

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

It seems like a radical approach to remove these Theia extensions without warning. Do I understand correctly that we could make the underlying vscode extensions available from elsewhere and just point to the new location from their package.json?

If later we want to deprecate them, it's fine, but with a bit of warning to adopters.

That being said, is there a quick fix that would permit CI to pass without removing the whole extensions?

@akosyakov
Copy link
Member

akosyakov commented Sep 5, 2019

@marcdumais-work all previous versions are still published to npmjs in forward compatible with 0.10.x

We can only exclude them from yarn workspaces that they are not built by yarn and not published but then why do we keep them in repo?

I agree that a record to CHANGELOG will be good to explain what happened and how to migrate.

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Sep 5, 2019

all previous versions are still published to npmjs in forward compatible with 0.x.x

True, but I'm concerned that when we release 0.11.0, apps that depend on the removed extensions, will pull version 0.10.0 of those dependencies as well : both removed extensions depend on "@theia/debug": "^0.10.0", which in turn depend on a bunch of other Theia extensions of that version:

    "@theia/application-package": "^0.10.0",
    "@theia/console": "^0.10.0",
    "@theia/core": "^0.10.0",
    "@theia/editor": "^0.10.0",
    "@theia/filesystem": "^0.10.0",
    "@theia/json": "^0.10.0",
    "@theia/languages": "^0.10.0",
    "@theia/markers": "^0.10.0",
    "@theia/monaco": "^0.10.0",
    "@theia/output": "^0.10.0",
    "@theia/preferences": "^0.10.0",
    "@theia/process": "^0.10.0",
    "@theia/terminal": "^0.10.0",
    "@theia/userstorage": "^0.10.0",
    "@theia/variable-resolver": "^0.10.0",
    "@theia/workspace": "^0.10.0",

If this happens, it will break such applications and it will be too late to change our mind. Though I'll admit I am not certain this will happen.

@marcdumais-work
Copy link
Contributor

We can only exclude them from yarn workspaces that they are not built by yarn and not published but then why do we keep them in repo?

Agreed - this "half-measure solution" does not make sense unless we intend to re-add the extensions in yarn workspace before next release (and it still potentially inconveniences adopters using next in the meantime as well).

Please see my alternative PR: #6120

@paul-marechal
Copy link
Member

Travis is not passing.

Also tried what @marcdumais-work described by simulating a 0.11 release without the removed extensions, using Verdaccio: If an IDE maker wants to depends on latest, it will pass the build step, but fail at runtime since both 0.10 and 0.11 packages will be pulled. Old applications using a lockfile won't be affected until they update their dependencies. Can be fixed by explicitly defining a resolutions field in package.json, but it kinda feels hacky, where you force yarn to not resolve versions by himself based on the different constraints (since they will be bogus).

Fixes #6111

Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
@svenefftinge
Copy link
Contributor Author

svenefftinge commented Sep 6, 2019

I have added the changes to yaml.lock and .travis.yml as well as a notice to the CHANGELOG.md.

@svenefftinge
Copy link
Contributor Author

There is not really a good reason to maintain these extensions further as there is a better way to use those debug adapters (through plugin system). So removing it is the right way.
Going through a deprecation phase would be nice if there were enough clients, and an actual way to inform them. But I don't think there is. Also, as Anton suggested, the existing versions are still there and working (not together with latest (after release), or next, though).

I suggest we do this for other extensions (e.g. java) eventually as well.

@marcdumais-work
Copy link
Contributor

@svenefftinge I am not against removal, but could we at the same time add the corresponding VS Code extensions to the example applications, so that adopters have a good example of how to go forward, if they want to keep the features?

As well we could document how one can use the resolutions block in their app's package.json to keep using the removed extensions with newer Theia releases, at least for a while.

Should we go ahead and deprecate these 2 extensions? Ideas how to document that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug adapters cannot be fetched
5 participants