-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
Define IIdentifiedSingleEditOperation.range as IRange instead of Range #90950
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.IRange
instead ofmonaco.Range
, thoughIIdentifiedSingleEditOperation
appears to be an exception in this regard, which means a strong dependency onmonaco.Range
is 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 #