Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

monaco: parseSnippet RangeError handling #12463

Merged

Conversation

FernandoAscencio
Copy link
Contributor

What it does

Closes #12434

  • Changes parseSnippets from recursive function to nested for-loops.
  • Eliminates 'prefix' requirement for JsonSerializedSnippet.is() as it is in VS Code.
  • Changes handling to account for missing 'prefix' parameter.
  • Add isFileTemplate param to Snippet to bring it in line with VSCode.
Notes

As discussed here there are three parameters in VS Code Snippet that are not present in Theia's: snippetSource, snippetIdentifier, and extensionId. Their inclusion or exclusion seems to be out of this PR's scope.

When adding a prefix to the error causing snippet as seen here it does not follow VS Code's display as seen here.

How to test

Follow the steps in #12434.

  1. Build the example application, browser or Electron, from the master branch
  2. Delete all extensions from folder ./plugins/. Unzip html-1.72.1.zip into it: html-1.72.1.zip
  3. Start example app. The error should not appear when loading.

Review checklist

Reminder for reviewers

@FernandoAscencio FernandoAscencio force-pushed the fa/parseSnippedRangeError branch from a9bf26a to a6a9825 Compare April 27, 2023 16:00
@vince-fugnitto vince-fugnitto added monaco issues related to monaco vscode issues related to VSCode compatibility labels Apr 27, 2023
FernandoAscencio and others added 2 commits April 27, 2023 14:22
Closes 12434

-Changes `parseSnippets` from recursive function to nested for-loops.
-Eliminates 'prefix' requirement for `JsonSerializedSnippet.is()`.
-Changes handling to account for missing 'prefix' parameter.
-Add `isFileTemplate` param to Snippet to bring it in line with VSCode

Signed-Off-By: FernandoAscencio <fernando.ascencio.cama@ericsson.com>
Co-authored-by: Paul Maréchal <paul.marechal@ericsson.com>
@vince-fugnitto vince-fugnitto force-pushed the fa/parseSnippedRangeError branch from ce1f830 to c89ff7c Compare April 27, 2023 18:22
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

I don't see the "maximum stack depth reached" error anymore, LGTM

@paul-marechal paul-marechal merged commit b7d4088 into eclipse-theia:master Apr 27, 2023
@vince-fugnitto vince-fugnitto added this to the 1.37.0 milestone Apr 27, 2023
jonah-iden pushed a commit to jonah-iden/theia that referenced this pull request May 2, 2023
Change `parseSnippets` from recursive function to nested for-loops.

Eliminate 'prefix' requirement for `JsonSerializedSnippet.is()`.

Change handling to account for missing 'prefix' parameter.

Add `isFileTemplate` param to Snippet to bring it in line with VS Code.

Signed-off-by: FernandoAscencio <fernando.ascencio.cama@ericsson.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
monaco issues related to monaco vscode issues related to VSCode compatibility
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

RangeError: Maximum call stack size exceeded with recent vscode.html builtin extension
3 participants