Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Expose CodeMirror instance and allow direct embedding of editor

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Debugger
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jlongster, Assigned: jlongster)

Tracking

unspecified
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

11 months ago
The editor.js API does not expose the internal CodeMirror instance, probably to try to keep it as internal as possible, but it's a hassle to try to abstract away the entire API. We are still inherently tied to CodeMirror: our API is very CodeMirror-inspired.

Our new debugger works directly with CodeMirror and uses properties that editor.js does not expose. Instead of continuing this cat-and-mouse, let's just expose CodeMirror. That will make it easy for tools like the new debugger to integrate with CodeMirror and use editor.js when it's in the panel.

Long-term I think everyone is on board for refactoring editor.js to take a different approach, where we still bundle in modes/themes/etc but do not try to abstract away the API. We will work on this over time.
(Assignee)

Updated

11 months ago
Summary: Expose CodeMirror instance from editor.js → Expose CodeMirror instance and allow direct embedding of editor
(Assignee)

Comment 1

11 months ago
Created attachment 8782539 [details] [diff] [review]
1295213.patch

Brian, you seem like a good reviewer for this. We need these changes for the new debugger. This allows us to avoid the whole iframe thing which makes things a lot simpler for us. It allows us to do what we want to CodeMirror, even style it, without touching anything in m-c. An iframe forces it to bring all the CSS needed with it and makes it really hard to modularize.
Attachment #8782539 - Flags: review?(bgrinstead)
(Assignee)

Updated

11 months ago
Assignee: nobody → jlong
Comment on attachment 8782539 [details] [diff] [review]
1295213.patch

Review of attachment 8782539 [details] [diff] [review]:
-----------------------------------------------------------------

Forwarding the review to gl, see some comments below

::: devtools/client/sourceeditor/editor.js
@@ +253,5 @@
> +   * Exposes the CodeMirror instance. We want to get away from trying to
> +   * abstract away the API entirely, and this makes it easier to integrate in
> +   * various environments and do complex things.
> +   */
> +  get codeMirror() {

We use `let cm = editors.get(this);` throughout this file.  We may as well switch those usages to this new getter so we go through the same code path and get error checking.

@@ +272,5 @@
>     * loads CodeMirror and all its dependencies.
>     *
>     * This method is asynchronous and returns a promise.
>     */
> +  appendTo: function (el, env, noIframe) {

I'll leave it up to :gl whether he thinks we should take on the complexity to allow a non-iframe host, but we should at least have a simple test case for this mode.

The main potential issues I see with this mode:
1: It's relying on the window's defined CodeMirror global which means there could be version mismatches if we aren't loading the same one that's in tree and we have a patched version to assist with l10n so I think there are assumptions in the code that we are using that
2: we hook things directly onto `win` (like `win.editor = this`) so if someone tried to load without iframe more than once bad things could happen
3: Certain options like this.config.themeSwitching = false would cause bad behavior (setting attributes on the top level window and not the editor itself)
4: It's just generally easier to keep track of bugs if everything is following the same code path.

I'm not saying we can't make the change, just listing some things we are taking on with it. I know it's harder to do custom styles in the editor and we are loading all of these chrome URIs so we'd need to convert these to something that could be loaded in a tab.

As for the arg itself, I think it's a little easier to follow if it's a positive statement like `useIframe = true`.  An alternative could also be to pass 'false' into env and we could check if `env === false` (as opposed to undefined) as a way to state you don't want an iframe.

@@ +495,5 @@
> +    }
> +    else {
> +      // If not loading in an iframe, the loading is synchronous and we don't
> +      // return a promise.
> +      onLoad();

I'd still be in favor of returning a promise that immediately calls onload and resolves just so the return value of this appendTo function doesn't change based on the args passed in
Attachment #8782539 - Flags: review?(bgrinstead) → review?(gl)
(Assignee)

Comment 3

11 months ago
(In reply to Brian Grinstead [:bgrins] from comment #2)
> @@ +272,5 @@
> >     * loads CodeMirror and all its dependencies.
> >     *
> >     * This method is asynchronous and returns a promise.
> >     */
> > +  appendTo: function (el, env, noIframe) {
> 
> I'll leave it up to :gl whether he thinks we should take on the complexity
> to allow a non-iframe host, but we should at least have a simple test case
> for this mode.
> 
> The main potential issues I see with this mode:
> 1: It's relying on the window's defined CodeMirror global which means there
> could be version mismatches if we aren't loading the same one that's in tree
> and we have a patched version to assist with l10n so I think there are
> assumptions in the code that we are using that

Not sure how we should enforce this, but it is still injecting CodeMirror and using the local one from m-c even if it's not loading in an iframe. Perhaps we should check if CodeMirror is already defined in the window and error to enforce this.

> 2: we hook things directly onto `win` (like `win.editor = this`) so if
> someone tried to load without iframe more than once bad things could happen

That's true. I should make sure to only load CodeMirror once.

> 3: Certain options like this.config.themeSwitching = false would cause bad
> behavior (setting attributes on the top level window and not the editor
> itself)
> 4: It's just generally easier to keep track of bugs if everything is
> following the same code path.
> 
> As for the arg itself, ...
> 
> @@ +495,5 @@
> > +    }
> > +    else {
> > +      // If not loading in an iframe, the loading is synchronous and we don't
> > +      // return a promise.
> > +      onLoad();
> 
> I'd still be in favor of returning a promise that immediately calls onload
> and resolves just so the return value of this appendTo function doesn't
> change based on the args passed in

I'm replying to all the above: perhaps it's just easier if I make a new method, and split out the important configuration bits that both the async and sync methods all. I don't want to enforce it to be async because there's no reason to, and every async API like this is another chance for integration tests to be flaky. But your review has convinced me that it's probably easier to just create a new method, and pull out the important bits of the loading code that configures CodeMirror into a single place. We really need to break up `appendTo` anyway

Comment 4

11 months ago
Comment on attachment 8782539 [details] [diff] [review]
1295213.patch

Clearing review to wait for the new version of the patch
Attachment #8782539 - Flags: review?(gl)
(In reply to James Long (:jlongster) from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > @@ +272,5 @@
> > >     * loads CodeMirror and all its dependencies.
> > >     *
> > >     * This method is asynchronous and returns a promise.
> > >     */
> > > +  appendTo: function (el, env, noIframe) {
> > 
> > I'll leave it up to :gl whether he thinks we should take on the complexity
> > to allow a non-iframe host, but we should at least have a simple test case
> > for this mode.
> > 
> > The main potential issues I see with this mode:
> > 1: It's relying on the window's defined CodeMirror global which means there
> > could be version mismatches if we aren't loading the same one that's in tree
> > and we have a patched version to assist with l10n so I think there are
> > assumptions in the code that we are using that
> 
> Not sure how we should enforce this, but it is still injecting CodeMirror
> and using the local one from m-c even if it's not loading in an iframe.
> Perhaps we should check if CodeMirror is already defined in the window and
> error to enforce this.

How are you importing the m-c codemirror?  It looks like CM is declared as an npm dependency: https://github.com/devtools-html/debugger.html/blob/4d962e8527a34b8726f182a5d138a9a6bf2a76dc/package.json#L32

Comment 6

11 months ago
This bug looks like it's actively being worked on by James for the new debugger, so I'm triaging this as P1.

Updated

11 months ago
Priority: -- → P1
(Assignee)

Updated

11 months ago
Blocks: 1295318
See Also: → bug 1292592
Status: NEW → ASSIGNED
(Assignee)

Comment 7

11 months ago
Created attachment 8788944 [details] [diff] [review]
1295213.patch

This is a much clearer patch. It extracts out the code that configures codemirror into its own function, and the `appendTo` function it left to deal with all the iframe stuff. I added an additional method `appendToLocalElement` for bypassing iframes entirely which just calls the configuration.
Attachment #8782539 - Attachment is obsolete: true
Attachment #8788944 - Flags: review?(gl)
Attachment #8788944 - Flags: feedback?(bgrinstead)
(Assignee)

Comment 8

11 months ago
(In reply to Brian Grinstead [:bgrins] from comment #5)
> 
> How are you importing the m-c codemirror?  It looks like CM is declared as
> an npm dependency:
> https://github.com/devtools-html/debugger.html/blob/
> 4d962e8527a34b8726f182a5d138a9a6bf2a76dc/package.json#L32

The local one is, but we have a local wrapper that conforms to the sourceeditor/editor.js interface and we replace that module with a call to `devtoolsRequire` when building for the panel which will require the m-c one: https://github.com/devtools-html/debugger.html/blob/master/webpack.config.devtools.js#L14
Comment hidden (mozreview-request)
Gabe, I pushed the patch to mozreview so it's a lot easier to review: https://reviewboard.mozilla.org/r/77262/diff/1#index_header

Comment 11

11 months ago
mozreview-review
Comment on attachment 8788949 [details]
Bug 1295213 - expose Codemirror instance and allow direct embedding in devtools

https://reviewboard.mozilla.org/r/77262/#review75514

::: devtools/client/sourceeditor/editor.js:357
(Diff revision 1)
>  
> -      // Create a CodeMirror instance add support for context menus,
> +    // Create a CodeMirror instance add support for context menus,
> -      // overwrite the default controller (otherwise items in the top and
> +    // overwrite the default controller (otherwise items in the top and
> -      // context menus won't work).
> +    // context menus won't work).
>  
> -      cm = win.CodeMirror(win.document.body, this.config);
> +    cm = win.CodeMirror(targetEl, this.config);

targetEl?  Should this be el?

Updated

11 months ago
Attachment #8788949 - Flags: review?(bgrinstead)
Comment on attachment 8788944 [details] [diff] [review]
1295213.patch

Review of attachment 8788944 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, see Comment 11
Attachment #8788944 - Flags: feedback?(bgrinstead) → feedback+
(Assignee)

Comment 13

11 months ago
(In reply to Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8788949 [details]
> Bug 1295213 - expose Codemirror instance and allow direct embedding in
> devtools
> 
> https://reviewboard.mozilla.org/r/77262/#review75514
> 
> ::: devtools/client/sourceeditor/editor.js:357
> (Diff revision 1)
> >  
> > -      // Create a CodeMirror instance add support for context menus,
> > +    // Create a CodeMirror instance add support for context menus,
> > -      // overwrite the default controller (otherwise items in the top and
> > +    // overwrite the default controller (otherwise items in the top and
> > -      // context menus won't work).
> > +    // context menus won't work).
> >  
> > -      cm = win.CodeMirror(win.document.body, this.config);
> > +    cm = win.CodeMirror(targetEl, this.config);
> 
> targetEl?  Should this be el?

Yep, I meant to say that I'm waiting on a clobber build so I haven't actually run this yet but wanted some early feedback on the approach.  I will post a new patch with any small fixes that need to be done, and I'll look into writing a test too.
(Assignee)

Comment 14

11 months ago
Created attachment 8788988 [details] [diff] [review]
1295213.patch

A few minor fixes. This will make the MozReview out-of-date, sorry... but you can see the general approach there. I use gecko-dev so I can't push to MozReview, need to set that up somehow.

I'm hoping a general pass will be good enough to r+ this. There isn't that much new code, but it just breaks up the `appendTo` function so we can access just the part we need.
Attachment #8788944 - Attachment is obsolete: true
Attachment #8788944 - Flags: review?(gl)
Attachment #8788988 - Flags: review?(gl)
Attachment #8788988 - Flags: feedback+

Comment 15

11 months ago
Comment on attachment 8788988 [details] [diff] [review]
1295213.patch

Review of attachment 8788988 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/sourceeditor/editor.js
@@ +250,5 @@
>    version: null,
>    config: null,
>    Doc: null,
>  
> +  /*

NIT: /** to be consistent.

@@ +320,2 @@
>  
> +  setup: function(el) {

This could use a JSDoc describing the setup process.
Attachment #8788988 - Flags: review?(gl) → review+
(Assignee)

Comment 16

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6defcca1ff4
(Assignee)

Comment 17

11 months ago
Created attachment 8789018 [details] [diff] [review]
1295213.patch

Updated w/nits addressed
Attachment #8788949 - Attachment is obsolete: true
Attachment #8788988 - Attachment is obsolete: true
Attachment #8789018 - Flags: review+

Comment 18

11 months ago
> +  appendToLocalElement(el) {
> +    this._setup(el);
> +  },

It might worth to use `appendToLocalElement: function(el) {` instead to keep method declaration format in consistency

Comment 19

11 months ago
Besides that, the origin `appendTo` method return a promise, therefore the caller could do something after load the codemirror.
https://dxr.mozilla.org/mozilla-central/search?q=editor.appendTo&redirect=false

If we want to apply `appendToLocalElement` to other component, return a promise can help `appendToLocalElement` become a drop replacement for `appendTo`.

Updated

11 months ago
Blocks: 1292592
(Assignee)

Comment 20

11 months ago
(In reply to Fred Lin [:gasolin] from comment #19)
> Besides that, the origin `appendTo` method return a promise, therefore the
> caller could do something after load the codemirror.
> https://dxr.mozilla.org/mozilla-central/search?q=editor.
> appendTo&redirect=false
> 
> If we want to apply `appendToLocalElement` to other component, return a
> promise can help `appendToLocalElement` become a drop replacement for
> `appendTo`.

There's no reason for it to be a promise though; it's synchronous. Just do whatever you were going to do right after the call to `appendToLocalElement`. If you need a promise you can wrap that call yourself. Making it a promise forces everything to be run on the next tick, which makes things annoying. Right now we initialize CodeMirror within a `componentDidMount` React lifecycle hook and it's incredibly nice to fully initialize everything synchronously.
(Assignee)

Comment 21

11 months ago
I guess we could just return `Promise.resolve()` but I'd rather do things right from the start. If you're changing some code from `appendTo` to `appendToLocalElement`, just remove the `.then` or `yield` or worst case wrap it in a promise yourself, which should be easy.
(Assignee)

Comment 22

11 months ago
Created attachment 8789410 [details] [diff] [review]
1295213.patch

Try run looks good, this is the patch I'm about to push. We can tweak it later if needed, but we really need this for turning on the new debugger today.
Attachment #8789018 - Attachment is obsolete: true
Attachment #8789410 - Flags: review+

Comment 23

11 months ago
Pushed by jlong@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/3a44273280cf
expose Codemirror instance and allow direct embedding in devtools r=bgrins,gl

Comment 24

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a44273280cf
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Comment 25

11 months ago
We do need a promise once we de-XUL and modify _setup to load files via appending links/scripts to DOM, we have to make sure scripts are loaded before doing the rest of operations. 

We can do it in another patch anyway.
(Assignee)

Comment 26

11 months ago
Synchronous is more ideal than async, so can you provide a way that I can just include the JS and CSS manually as link/script tags? If there was a single JS file to include, then I could assume it's loaded and just use a sync API. But I haven't really thought through it.
If we were able to do F) in https://bugzilla.mozilla.org/show_bug.cgi?id=1292592#c40 it could be sync without needing to load script tags but I'm not sure it would work with our loader.  Another alternative as in Comment 26 is to bundle up our codemirror assets into a single script tag that could be included on any document that wants to append to local element.  I'd prefer the former if it's possible.

Comment 28

11 months ago
There are bunch of CodeMirror related scripts need to be loaded before we call `win.CodeMirror` or `cm` in editor.js
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/sourceeditor/editor.js#L53

Without XUL's `Services.scriptloader.loadSubScript` we might need to use `promise.all` to make sure all scripts are loaded before running the rest of setup process, or we'll get `win.CodeMirror not defined` error. 

Put dependency js/css in html file might be more easier without introducing the async process. But we will need to maintain the js/css list in each component which refers the sourceeditor. Bundle them into a single script tag might help.

Updated

11 months ago
Depends on: 1301788
You need to log in before you can comment on or make changes to this bug.