1295213 - Expose CodeMirror instance and allow direct embedding of editor

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[James Long (:jlongster)]

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.

Comment 1

[James Long (:jlongster)]

Attachment: 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.

Comment 2

[Brian Grinstead [:bgrins]]

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

Comment 3

[James Long (:jlongster)]

(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

[Gabriel Luong [:gl] (ΦωΦ)]

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

Clearing review to wait for the new version of the patch

Comment 5

[Brian Grinstead [:bgrins]]

(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

[Eddy Bruel [:ejpbruel]]

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

Comment 7

[James Long (:jlongster)]

Attachment: 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.

Comment 8

[James Long (:jlongster)]

(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 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1295213 - expose Codemirror instance and allow direct embedding in devtools

Review commit: https://reviewboard.mozilla.org/r/77262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77262/

Comment 10

[Brian Grinstead [:bgrins]]

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

[Brian Grinstead [:bgrins]]

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?

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8788944 [details] [diff] [review]
1295213.patch

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

This looks fine, see Comment 11

Comment 13

[James Long (:jlongster)]

(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.

Comment 14

[James Long (:jlongster)]

Attachment: 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.

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 16

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6defcca1ff4

Comment 17

[James Long (:jlongster)]

Attachment: 1295213.patch

Updated w/nits addressed

Comment 18

[[e-mail]]

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

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

Comment 19

[[e-mail]]

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`.

Comment 20

[James Long (:jlongster)]

(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.

Comment 21

[James Long (:jlongster)]

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.

Comment 22

[James Long (:jlongster)]

Attachment: 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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3a44273280cf
expose Codemirror instance and allow direct embedding in devtools r=bgrins,gl

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a44273280cf

Comment 25

[[e-mail]]

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.

Comment 26

[James Long (:jlongster)]

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.

Comment 27

[Brian Grinstead [:bgrins]]

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

[[e-mail]]

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.