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

re-add DSPLaunchDelegate.createDebugTarget(...) as deprecated method #654

Closed
wants to merge 1 commit into from

Conversation

sebthom
Copy link
Member

@sebthom sebthom commented May 23, 2023

The signature of the public API method DSPLaunchDelegate.createDebugTarget was changed by 1e52445 resulting in compile / linkage errors in our projects where we need to override it.

This PR re-adds the method with the old signature as deprecated to allow graceful API transition phase.

* return target;
*/
final var supplier = streamsSupplier.get();
return createDebugTarget(subMonitor, supplier::close, supplier.in, supplier.out, launch, dspParameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

The newer/recommended method should not reference the deprecated one, it would better be the other way round.

Copy link
Member Author

Choose a reason for hiding this comment

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

That does not work since the new createDebugTarget method is invoked by public void launch(DSPLaunchDelegateLaunchBuilder builderSrc) of the same class. I would then have to revert all your changes to this class.

If I make the deprecated method delegate to the new method it will effectively never be called, thus overriding it will be without any effect.

@mickaelistria
Copy link
Contributor

The code is not functionally equivalent, while with the current master eclipse-wildwebdeveloper/wildwebdeveloper#1174 does work decently (I can debug with vscode-js-debug), this patch leads to a failure on server-side. I suspect one issue is that the Debug target is then not using the correct streams (ie it's trying to keep using the same main stream while a new socket connection to the given port must be made).

@sebthom
Copy link
Member Author

sebthom commented May 23, 2023

I am out of ideas how to solve this in a backwards compatible way then.

@mickaelistria
Copy link
Contributor

Unfortunately I don't see an easy way either, the only viable one would be that you copy DSPLaunchDelegate in your code... I think overall, the simpler and most profitable path for everyone (including you) is to just drop backward compatibility and set narrow version ranges to avoid runtime errors. Would it help if we set the bundle version to 0.16.0 instead of 0.15.2 (I don't have high hopes it makes things really easier, but if can facilitate things, it's also an easy change for LSP4E)

@sebthom
Copy link
Member Author

sebthom commented May 23, 2023

Right now it makes no difference for us if it will be a 0.16.0 or 0.15.2.

In the future, the best would be of course if the project follows semver. But I think it would already help if the project commits to always increment the minor version for bundles that introduce backward incompatible changes and only increments the patch version if binary compatibility is guaranteed.

@sebthom sebthom closed this May 24, 2023
@sebthom sebthom deleted the createDebugTarget branch October 16, 2023 17:42
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.

None yet

2 participants