Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Brackets throws exceptions on code with Mustache templates #2906

Closed
peterflynn opened this issue Feb 20, 2013 · 13 comments
Closed

Brackets throws exceptions on code with Mustache templates #2906

peterflynn opened this issue Feb 20, 2013 · 13 comments
Assignees
Milestone

Comments

@peterflynn
Copy link
Member

Not sure if this regressed at the original CMv3 merge, or in the more recent pull #2904.

First bug:

  1. Paste this code into an HTML file:
<!doctype html>
<html>
<head>
    <script type="text/x-mustache-template" id="myTemplate">
        <h1>{{firstName}} {{lastName}}</h1>
        <p>Blog URL: <a href="{{blogURL}}">{{blogURL}}</a></p>
    </script>

</head>
<body></body>
</html>

Result: the second line of the template does not appear to be highlighted as HTML.

Second bug:

  1. Install the JS code hints extension
  2. In the same file as above, put your cursor on the end of the first template line and press "."

Result: exception thrown in JS code hinting code. Looking in the debugger, CM thinks the mode at that position is "javascript". (It should be "html").

Third bug:
Start with this HTML code:

<!doctype html>
<html>
<head>
    <script type="text/x-mustache-template" id="myTemplate">
    </script>

</head>
<body></body>
</html>

And put this code on your clipboard:

<h1>{{firstName}} {{lastName}}</h1>
<p>Blog URL: <a href="{{blogURL}}">{{blogURL}}</a></p>
  1. Put your cursor at the start of the line containing </script>
  2. Paste
  3. Select both the pasted lines and hit Tab to indent

Result: exception thrown in CodeHintManager. At this point, apparently the mode is null. (I assume it should be "html" here too -- is null ever a valid mode?).

@peterflynn
Copy link
Member Author

One other thought @njx... it seems like Editor.getModeForSelection() changing to sometimes return an object could potentially break a lot of people. In fact I think at one point on the v3 branch the API changed in a similar way by accident and a whole bunch of bugs cropped up as a result (hence the code now returning .name in the mixed-mode case). It seems less disruptive to have getModeForSelection() continue to always just return the string name, and if someone needs the full mode config object we could add a flag or a new API for that.

@njx
Copy link

njx commented Feb 20, 2013

FWIW, I didn't actually change Editor.getModeForSelection(); I just changed the case of the type annotation in its comment (which already indicated that it could return either an object or a string) while I was updating comments elsewhere. I did change a case in EditorUtils.getModeFromFileExtension() to return an object instead of a string, but there was already another case in there that did so.

That said, I agree that it would make more sense to actually change Editor.getModeForSelection() to always return a string. Looking at the code, it seems like the only case where it could return an object is in the non-mixed case, where it returns _codeMirror.getOption("mode") (assuming that CM returns a mode object in that case if one was passed in). (I think the other cases that set a mode should probably continue to be able to take either version.) But in any case, I'm not sure that's relevant to this bug.

@ghost ghost assigned njx Feb 20, 2013
@njx
Copy link

njx commented Feb 20, 2013

To me for sprint 21.

@peterflynn
Copy link
Member Author

Right, sorry -- I didn't explain it very well. I think what's causing trouble is the change in EditorUtils. Previously all the "core" non-mixed modes were strings, so it was hard to hit cases where Editor.getModeForSelection() returned an object (JSON, and I suppose C/C++ et al... but that was it). Now that HTML files return an object mode, it's much easier to run into.

@njx
Copy link

njx commented Feb 20, 2013

Actually, all of these bugs reproduce in CMv2 as well (e.g. in dce04b6). It looks like the hack to make htmlmixed stay in HTML mode for the contents of a <script> tag with a mustache/handlebars type never actually worked properly if there was more than one direct child tag of the <script>. Reducing to medium priority since this isn't a regression.

@redmunds, do you think you could look at this? It seems like just leaving the mode untouched initially doesn't work--there must be something else we have to do to get the html mode to "stick" for subsequent tags within the <script>.

@ghost ghost assigned redmunds Feb 20, 2013
@peterflynn
Copy link
Member Author

Hmm, weird -- I could swear the very earliest implementation worked since Mike & Ryan were using it... but even back in Sprint 14 it appears broken. Sorry for the confusion!

@njx
Copy link

njx commented Feb 20, 2013

It might not have been that likely to hit since it's probably pretty common for a template to just consist of one tag (plus its descendants).

@redmunds
Copy link
Contributor

I'm seeing the same bug in the CodeMirror htmlmixed demo page, so I opened a CodeMirror bug, to get Marijn's advice on how best to fix.

@redmunds
Copy link
Contributor

Marijn checked in a fix for this one: codemirror/codemirror5@c69ce2e

@njx
Copy link

njx commented Feb 21, 2013

Updated to latest CM master (which happens to be 3.1). FBNC @peterflynn

@redmunds
Copy link
Contributor

There's also a change required on our side: #2929 This is not required. I was using wrong codemirror aha.

@pthiess
Copy link
Contributor

pthiess commented Feb 25, 2013

Reviewed.

@peterflynn
Copy link
Member Author

Confirmed fixed. And I believe the language API changes resolve my concerns about Editor.getModeForSelection() returning a non-string sometimes. The new API effectively guarantees that modes are always specified as a string (which may be a mimetype alias pointing to a config object -- but the mode itself is never a config object directly anymore).

Closing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants