Define IIdentifiedSingleEditOperation.range as IRange instead of Range#90950
Define IIdentifiedSingleEditOperation.range as IRange instead of Range#90950alexdima merged 2 commits intomicrosoft:masterfrom
Conversation
With the upcoming changes in TypeScript 3.8 to support `import type`, it is even more compelling to declare weak references via types rather than strong references via concrete classes. Most APIs in Monaco already do this well, accepting `monaco.IRange` instead of `monaco.Range`, though `IIdentifiedSingleEditOperation` appears to be an exception in this regard, which means a strong dependency on `monaco.Range` is required to use `ITextModel.pushEditOperations()`. It would be nice to relax this constraint, though I admit I have not yet attempted to trace through whether this breaks anything since I am submitting this via GitHub's web UI.
|
Indeed, the test failures illustrate where VS Code/Monaco expect a return undoEdits.map(edit => Selection.fromPositions(edit.range.getEndPosition()));Would you be open to changes like function getEndIPosition(range: IRange): IPosition {
if (range instanceof Range) {
return range.getEndPosition();
} else {
return {
lineNumber: range. endLineNumber,
column: range. endColumn,
};
}
}I'm happy to go through and try to do these sorts of refactorings, but if you aren't open to this change, then feel free to close the PR. |
|
Thank you! |
|
w00t!!! Thanks @alexdima for taking my proposal and making it legit! |
| /** | ||
| * The range to replace. This can be empty to emulate a simple insert. | ||
| */ | ||
| range: Range; |
There was a problem hiding this comment.
@alexdima Did you intend for this to be a Range rather than an IRange? Without tracing through the code, I assume it's extra work to relax this constraint right now?
There was a problem hiding this comment.
That is intentional. You can see how IValidEditOperation is always returned from the editor or passed in as a callback argument by the editor, so this is not a constrained on the editor user, it is a constrained on the editor implementation.
|
Also, @alexdima I'm not sure how much work it is on your end to publish a new In the meantime, I'll guess I'll try following the instructions on https://github.com/microsoft/monaco-editor/blob/master/CONTRIBUTING.md to produce a local build. |
With the upcoming changes in TypeScript 3.8 to support
import type, it is even more compelling to declare weak references via types rather than strong references via concrete classes. Most APIs in Monaco already do this well, acceptingmonaco.IRangeinstead ofmonaco.Range, thoughIIdentifiedSingleEditOperationappears to be an exception in this regard, which means a strong dependency onmonaco.Rangeis required to useITextModel.pushEditOperations().It would be nice to relax this constraint, though I admit I have not yet attempted to trace through whether this breaks anything since I am submitting this via GitHub's web UI.
This PR fixes #