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

SetModelMarkers shows "Undefined" as error message #790

Closed
valarGuru opened this issue Mar 28, 2018 · 21 comments
Closed

SetModelMarkers shows "Undefined" as error message #790

valarGuru opened this issue Mar 28, 2018 · 21 comments

Comments

@valarGuru
Copy link

valarGuru commented Mar 28, 2018

Hi,
I am using "setModelMarkers()" to highlight the syntax errors in Monaco editor for my custom language.
The message added to the marker is not getting displayed on hovering over the highlighted text.

{
                code: null, 
                source: null,
                severity: 3,
                startLineNumber: 1,
                startColumn: 1,
                endLineNumber: 1,
                endColumn: 4 + 1,
                message: 'Syntax error\n'
 }

The message always shows as "undefined".
image
Please let me know if any thing is missed or done wrong.

monaco-editor version: 0.11.1
Browser: Chrome
OS: win 7

@valarGuru valarGuru changed the title SerModelMarkers "Undefined" error message SetModelMarkers shows "Undefined" as error message Mar 28, 2018
@alexdima
Copy link
Member

This works fine for me:

var ed = monaco.editor.create(document.getElementById("container"), {
	value: "function hello() {\n\talert('Hello world!');\n}",
	language: "javascript"
});

monaco.editor.setModelMarkers(ed.getModel(), 'test', [{
    startLineNumber: 2,
    startColumn: 1,
    endLineNumber: 2,
    endColumn: 1000,
    message: "a message",
    severity: monaco.Severity.Warning
}])

image

@valarGuru
Copy link
Author

valarGuru commented Apr 2, 2018

I tried the same, still the message says "undefined".

let myDiv = this.editorContent.nativeElement;
        this.editor  = monaco.editor.create(myDiv, {
            value: "function hello() {\n\talert('Hello world!');\n}",
            language: "javascript"
        });
        monaco.editor.setModelMarkers(this.editor.getModel(), 'test', [{
            startLineNumber: 2,
            startColumn: 1,
            endLineNumber: 2,
            endColumn: 1000,
            message: "a message",
            severity: monaco.Severity.Warning
        }])

image

Trying the same with Monaco playground working fine.
I am using Monaco editor with the angular 4. Do all these issues occur due to any incompatibility or could there be any other possible reasons?.

@ryanlin1986
Copy link

I have same issue to, after upgrading to latest manco version.

@rcjsuen
Copy link
Contributor

rcjsuen commented Apr 4, 2018

@ryanlin1986 Are you also using Angular 4?

@taras-mytofir
Copy link

I have similar "undefined" issue with a bit different steps but I think root cause is the same.

This example from monaco playground when running from Angular 5 application produces "undefined" when you hover over warning on enum value, other warnings like "Missing property XXX" are affected too.

Issue is happening when signalInnerHTML is executed which triggers Promise.all resolving above it.

But the issue is actually on Angular's Zone.js side as it re-defines promises. It has to be fixed in Promise.all definition, index/count mismatch is happening in monaco-editor case.

The only workaround I have for now is by redefining Promise.all with a fix locally (e.g. within polyfills.ts)

Promise.all = function (values: any): Promise<any> {
    let resolve: (v: any) => void;
    let reject: (v: any) => void;
    let promise = new this((res, rej) => {
      resolve = res;
      reject = rej;
    });
    let count = 0;
    let index = 0;
    const resolvedValues: any[] = [];
    for (let value of values) {
      if (!(value && value.then)) {
        value = this.resolve(value);
      }
      value.then(
          ((index) => (value: any) => {
            resolvedValues[index] = value;
            count--;
            if (!count) {
                resolve(resolvedValues);
            }
          })(index),
          reject);
      count++;
      index++;
    }
    if (!count) resolve(resolvedValues);
    return promise;
}

philippguertler pushed a commit to gentics/mesh-ui that referenced this issue Apr 4, 2018
@ryanlin1986
Copy link

@rcjsuen Hi, I'm not using angular at all, just plain javascript

@rcjsuen
Copy link
Contributor

rcjsuen commented Apr 4, 2018

@ryanlin1986 Can you reproduce the problem in the playground?

@valarGuru
Copy link
Author

@ryanlin1986 try using Monaco editor version 0.10.1. The "undefined" issue is there from version 0.11.0.

@ryanlin1986
Copy link

@rcjsuen turns out it is because of the Promise polyfills, same as angular issue.
Solved by myself now. Thanks for the response.
(I believe the previous version works without issue)

@taras-mytofir
Copy link

Previous version of monaco-editor doesn't have this issue because there was no signalInnerHTML routine there. Monaco-editor can actually do proactive check to don't have this issue by verifying if strValue is truthy here.

@alexdima
Copy link
Member

alexdima commented Apr 10, 2018

@taras-mytofir Is this an issue we should file against Angular's Zone.js. I think Promise.all should return resolved promises in the order they were passed in.

@taras-mytofir
Copy link

@alexandrudima I wanted to do that but wasn't able to prepare inline example quickly which would illustrate this issue as some specific promises are created inside monaco and I didn't have time to further dig into the monaco-editor's code. Some example like here (this one works fine) would be good.

If somebody understands better what exactly is happening inside monaco-editor's case this is angular 5 plunkr app which can be used to prepare such inline example using developer tools console as promises there are Zone.js' ones.

I think after we have such example we can create issue on Zone.js side.

@alexdima
Copy link
Member

@taras-mytofir Here is the repro

https://plnkr.co/edit/UO2QhIa5eeLm6tPAyAjt?p=preview

VSCode historically used WinJS Promises... and we are slowly moving to the standard Promise, sometimes by using Promise.all ...

  let p1 = WinJSPromise.as(3);
  let p2 = Promise.resolve(45);
  Promise.all([p1, p2]).then(res => {
    console.log(`Promise.all: `, res);
  })
  WinJSPromise.join([p1, p2]).then(res => {
    console.log(`WinJSPromise.join: `, res);
  });

The output I get from Promise.all is [45], where I would have expected [3, 45], like the one from WinJSPromise.join. I suspect instanceof is used in the Promise.all implementation instead of checking for a then method...

@buinhansinh
Copy link

@taras-mytofir . Can you suggest a solution to work around this ? . I am using monaco-edtior version 0.13 with angular 5 and face the same issue

@taras-mytofir
Copy link

@buinhansinh I mentioned my workaround in this comment.
There is other workaround mentioned in linked issue but I have not tried it myself.

@buinhansinh
Copy link

@taras-mytofir , is it possible to use Line Decorator instead of Model Markers ? Decorators

@DenysVuika
Copy link

I've spent days trying to figure out why JSON validation messages won't work in Angular. Thanks @taras-mytofir for the #790 (comment). Helps a lot as a short term solution.

@jvalkeal
Copy link

Have to give people kudos debugging this stuff out. I wonder if fixed microsoft/vscode#53526 eventually gives working monaco without these workarounds?

fcastill added a commit to project-flogo/flogo-web that referenced this issue Aug 30, 2018
Error was caused due to discrepancies between zone.js and monaco editor Promise.all implementation. Added polyfill patch for short term solution.

Track issues:
- microsoft/monaco-editor#790
- angular/zone.js#1077
fcastill added a commit to project-flogo/flogo-web that referenced this issue Aug 30, 2018
Error was caused due to discrepancies between zone.js and monaco editor Promise.all implementation. Added polyfill patch for short term solution.

Track issues:
- microsoft/monaco-editor#790
- angular/zone.js#1077
@tnclark8012
Copy link
Member

@jrieken I see that removal of WinJS.Promise is an epic you're working on. Would you be open to a PR fixing this issue specifically? Changing MarkdownRenderer.getOptions' codeBlockRenderer to return a native promise? Or perhaps ModeServiceImpl._onReady to use Promise.resolve(true) instead of WinJS.Promise.as(true) ?

@jrieken
Copy link
Member

jrieken commented Sep 13, 2018

Sure, go ahead but try to keep it small or contained to one area.

mhevery pushed a commit to angular/zone.js that referenced this issue Oct 26, 2018
For ZoneAwarePromise.all, as [implemented](https://github.com/angular/zone.js/blob/7201d44451f6c67eac2b86eca3cbfafde14035d6/lib/common/promise.ts), the `count` variable serves three purposes:

1.  Count the number of values passed-in
2.  Specify the index of a resolved value in `resolvedValues`
3.  Track when all the promises have been resolved.

In the event that `value.then` is immediately called, `count` will be decremented at the incorrect time resulting in a potentially negative value or reaching 0 when not all values have actually been resolved. 

The fix is to be more meticulous about using the correct indices at the correct time and to not overload the count and number of resolved promises.  This updated version is more explicit about the purpose of the `unresolvedCount` and `valueIndex` variables.  `unresolvedCount` needs to start at 2 to prevent `resolve` from being called too soon in the event that `value.then` calls the callback immediately. The scoping of the index for use in `resolvedValues` is made constant to guarantee the correct index is used.

This buggy behavior specifically manifested as an issue in the Monaco editor but most likely occurs elsewhere as well in cases where promises may immediately call into `.then`.

Related issue:
microsoft/monaco-editor#790 (comment)
@alexdima
Copy link
Member

We have now migrated to native promises entirely.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants