1239018 - Implement file watching and live reloading

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[James Long (:jlongster)]

The toolbox needs to watch for file changes and broadcast them out. An iframe instance can listen for these changes and handle them, and implement hot reloading for various kinds of modules.

This bug is just for the lower-level bits to watch files in the toolbox. I'll file other bugs for implement hot module reloading in the frontend by listening to these changes.

Comment 1

[James Long (:jlongster)]

If I watch all JS and CSS files in devtools/client, there are 2293 files, and it takes about 86ms to do a `stat` on all of them. Not bad, and it's fine since it runs in a worker.

Comment 2

[James Long (:jlongster)]

Attachment: 1239018.patch

I'm sure you have some ideas about better ways to do this. The biggest problem is that, for reloading individual modules, we can't just load any arbitrary file from a file:// URL. We need to reload the actual resource:// URL because, particularly for JS, there are relative includes inside the file. And if include a file:// JS module, the relative includes are going to pull in a whole new fresh tree because it's not using the right resource:// paths that are already in the loader.

That means that when a file is changed, we need to map it back to a resource:// URL (or a chrome:// one) and reload that URL. The BrowserLoader implements this by deleting it from the loader module cache and then re-requiring it.

This means it won't work on systems that don't use symlinks though, because this assumes that re-requiring a resource:// URL is going to grab your local contents.

A potential fix for no-symlink systems is to manually copy the file into the obj-* directory ourselves. That wouldn't be too bad, and I think we are allowed some hacks because this is all for development!

Comment 3

[James Long (:jlongster)]

Attachment: 1239018.patch

Better patch, doesn't descend into test dirs (only 463 files and takes ~17ms now), and styling fixes.

Comment 4

[James Long (:jlongster)]

Attachment: 1239018.patch

Forgot to include the preferences file.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8707045 [details] [diff] [review]
1239018.patch

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

::: devtools/client/framework/file-watcher-worker.js
@@ +53,5 @@
> +    }
> +
> +    const lastTime = modifiedTimes.get(file);
> +
> +    if (info.lastModificationDate.getTime() > lastTime) {

Does a folder's lastModificationDate include the max of all children?  If so, you could skip checking all of it's children when the folder time is the same.

Just a potential optimization... seems like it's probably already fast enough for beta use.

::: devtools/client/framework/file-watcher.js
@@ +14,5 @@
> +  // built directory, and then we strip off the obj-* directory to
> +  // point at the directory where the built obj directory lives. This
> +  // assumes Firefox is running a local build. Lastly, we strip off
> +  // the file:// extension.
> +  localBaseURI = resolveResourceURI(baseURI)

I think this is okay for now.

We can refine this by using either the "devtools.loader.srcdir" pref (if set) and / or info from :ochameau's add-on approach, both of which know where the real source checkout is (because the user has effectively told them).  So, that would get it working even on Windows.

@@ +34,5 @@
> +    }
> +    onFileChanged("resource://" + localURI);
> +  }
> +
> +  watchWorker.postMessage({ path: localBaseURI + '/devtools/client' });

Doesn't `localBaseURI` already include devtools/client...?  `baseURI` passed to `watchFiles` does at least...  Maybe it's getting stripped though...?

@@ +38,5 @@
> +  watchWorker.postMessage({ path: localBaseURI + '/devtools/client' });
> +  return watchWorker;
> +}
> +
> +this.EXPORTED_SYMBOLS = ["watchFiles"];

Why is this a JSM style module?  Can't you use CommonJS and the DevTools loader to require it?

::: devtools/client/framework/toolbox.js
@@ +7,5 @@
>  const MAX_ORDINAL = 99;
>  const ZOOM_PREF = "devtools.toolbox.zoomValue";
>  const SPLITCONSOLE_ENABLED_PREF = "devtools.toolbox.splitconsoleEnabled";
>  const SPLITCONSOLE_HEIGHT_PREF = "devtools.toolbox.splitconsoleHeight";
> +const HOTRELOAD_PREF = "devtools.hotreload";

This should probably be more specific, like "devtools.loader.hotreload", but that may depend on what module it ultimately lives in.

@@ +442,5 @@
>        if (DevToolsUtils.testing) {
>          yield performanceFrontConnection;
>        }
>  
> +      if (Services.prefs.getBoolPref(HOTRELOAD_PREF)) {

I feel like this is a loader feature, isn't it?  Seems somewhat random in the toolbox.  Is there a reason for it to be here instead?

I am thinking esp. of features outside the toolbox... like I'll be working on responsive design and would want to use this, but it's not part of the toolbox directly.

@@ +445,5 @@
>  
> +      if (Services.prefs.getBoolPref(HOTRELOAD_PREF)) {
> +        const { watchFiles } = Cu.import('resource://devtools/client/framework/file-watcher.js', {});
> +        this._fileWatcher = watchFiles("resource://devtools/client", changedFile => {
> +          this.emit("file-changed", changedFile);

Nothing in this patch uses "file-changed", but I guess this bug is just adding the event to be used later?

Comment 6

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Comment on attachment 8707045 [details] [diff] [review]
> 1239018.patch
> 
> Review of attachment 8707045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/framework/file-watcher-worker.js
> @@ +53,5 @@
> > +    }
> > +
> > +    const lastTime = modifiedTimes.get(file);
> > +
> > +    if (info.lastModificationDate.getTime() > lastTime) {
> 
> Does a folder's lastModificationDate include the max of all children?  If
> so, you could skip checking all of it's children when the folder time is the
> same.
> 
> Just a potential optimization... seems like it's probably already fast
> enough for beta use.

Unfortunately not (that would cause a bunch of IO whenever modifying a file, since it has to walk up all parents to the root). But yeah, it seems pretty fast and we do it in a worker anyway.

> 
> ::: devtools/client/framework/file-watcher.js
> @@ +14,5 @@
> > +  // built directory, and then we strip off the obj-* directory to
> > +  // point at the directory where the built obj directory lives. This
> > +  // assumes Firefox is running a local build. Lastly, we strip off
> > +  // the file:// extension.
> > +  localBaseURI = resolveResourceURI(baseURI)
> 
> I think this is okay for now.
> 
> We can refine this by using either the "devtools.loader.srcdir" pref (if
> set) and / or info from :ochameau's add-on approach, both of which know
> where the real source checkout is (because the user has effectively told
> them).  So, that would get it working even on Windows.

Oh, good point.

> 
> @@ +34,5 @@
> > +    }
> > +    onFileChanged("resource://" + localURI);
> > +  }
> > +
> > +  watchWorker.postMessage({ path: localBaseURI + '/devtools/client' });
> 
> Doesn't `localBaseURI` already include devtools/client...?  `baseURI` passed
> to `watchFiles` does at least...  Maybe it's getting stripped though...?

It doesn't include it, but you're right that it's a bug. The hack for moving from built files to source files by stripping off the `obj-*` path and everything after it forces it to resolve to the root `gecko-dev` folder (or whatever it's called). Thus ignoring any subfolder that was passed in with `baseURI`.

I'll think more about this.

> 
> @@ +38,5 @@
> > +  watchWorker.postMessage({ path: localBaseURI + '/devtools/client' });
> > +  return watchWorker;
> > +}
> > +
> > +this.EXPORTED_SYMBOLS = ["watchFiles"];
> 
> Why is this a JSM style module?  Can't you use CommonJS and the DevTools
> loader to require it?


When I do that, I get `ChromeWorker` is undefined. I just looked at the loader.js code though and I see that you have to import it like `const { ChromeWorker } = require('chrome')`. I'll change it.

> 
> ::: devtools/client/framework/toolbox.js
> @@ +7,5 @@
> >  const MAX_ORDINAL = 99;
> >  const ZOOM_PREF = "devtools.toolbox.zoomValue";
> >  const SPLITCONSOLE_ENABLED_PREF = "devtools.toolbox.splitconsoleEnabled";
> >  const SPLITCONSOLE_HEIGHT_PREF = "devtools.toolbox.splitconsoleHeight";
> > +const HOTRELOAD_PREF = "devtools.hotreload";
> 
> This should probably be more specific, like "devtools.loader.hotreload", but
> that may depend on what module it ultimately lives in.

Yeah, not sure about it. We can decide on it later after this patch goes through a few revisions.

> 
> @@ +442,5 @@
> >        if (DevToolsUtils.testing) {
> >          yield performanceFrontConnection;
> >        }
> >  
> > +      if (Services.prefs.getBoolPref(HOTRELOAD_PREF)) {
> 
> I feel like this is a loader feature, isn't it?  Seems somewhat random in
> the toolbox.  Is there a reason for it to be here instead?
> 
> I am thinking esp. of features outside the toolbox... like I'll be working
> on responsive design and would want to use this, but it's not part of the
> toolbox directly.

I thought it was a loader feature as well, until I realized that because we use iframes we have multiple loaders. So the debugger and the memory tool would start 2 file watchers. We could try to limit the files that they watch, so each loader watches only the files it's interested in, but I want to also watch the shared directory because there's no reason that can't be hot reloaded as well.

I found it easier to just have the toolbox emit file changed events, and any number of things can listen to them.

What we could do is BrowserLoader could start the file watching service when the toolbox isn't available. Currently it uses special APIs to get the current toolbox, but we should guard against that anyway. We could check that if there is no toolbox, we start the watching service ourselves.

> 
> @@ +445,5 @@
> >  
> > +      if (Services.prefs.getBoolPref(HOTRELOAD_PREF)) {
> > +        const { watchFiles } = Cu.import('resource://devtools/client/framework/file-watcher.js', {});
> > +        this._fileWatcher = watchFiles("resource://devtools/client", changedFile => {
> > +          this.emit("file-changed", changedFile);
> 
> Nothing in this patch uses "file-changed", but I guess this bug is just
> adding the event to be used later?

Right, bug 1239063 implements the other end in the BrowserLoader to react to file changes and actually do the hot reloading.

Comment 8

[James Long (:jlongster)]

Attachment: 1239018.patch

Fixed up everything from above. Also merging the bug that augments BrowserLoader with this one, since the BrowserLoader also can fire up a watcher now.

Ryan, I made it so that if a toolbox doesn't exist, BrowserLoader automatically fires up its own watcher. I think this should work? Is that code right? Also, I don't know when to terminate the watcher in BrowserLoader. Should I listen to an event on the tab or something?

Comment 9

[James Long (:jlongster)]

I also need to only include ReactProxy.js if DEBUG_JS_MODULES is on.

Comment 10

[James Long (:jlongster)]

Attachment: 1239018.patch

Fixed some errors from quick refactoring

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8708025 [details] [diff] [review]
1239018.patch

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

::: devtools/client/preferences/devtools.js
@@ +16,5 @@
>  
>  // Disable the error console
>  pref("devtools.errorconsole.enabled", false);
>  
> +// Developer prefs

Nit: Pretty ambiguous since all the tools are for developers... maybe "DevTools development workflow" or something.

::: devtools/client/shared/browser-loader.js
@@ +51,5 @@
>      sharedGlobal: true,
>      sandboxPrototype: window,
>      paths: Object.assign({}, loaderOptions.paths),
>      invisibleToDebugger: loaderOptions.invisibleToDebugger,
> +    requireHook: (id, require) => {

Hmm, why does this name change?  Isn't `require` the correct name?

@@ +63,5 @@
>        }
>  
>        return require(uri);
> +    },
> +    exportsHook: (uri, exports, require) => {

Is this a loader option?  I see no references to `exportsHook`.

@@ +93,5 @@
> +    const { TargetFactory } = devtools.require("devtools/client/framework/target");
> +    const { gDevTools } = devtools.require("resource://devtools/client/framework/gDevTools.jsm");
> +
> +    function onFileChanged(fileURI) {
> +      dump('Changed: ' + fileURI + '\n');

Use " everywhere

@@ +128,5 @@
> +    if(TargetFactory.isKnownTab(tab)) {
> +      const target = TargetFactory.forTab(tab);
> +      const toolbox = gDevTools.getToolbox(target);
> +
> +      toolbox.on("file-changed", (_, fileURI) => {

I am still not sure that the watching should be in the toolbox.  So far, the only code that watches "file-changed" events in the BrowserLoader, but then you have the entire toolbox emitting events.  If I don't open a tool that uses BrowserLoader, the toolbox will still be emitting events, but no one will listen, even though I could probably be using at least the CSS part.

Also, if it is in the toolbox, what happens with 2 toolboxes open in different tabs?  Spinning up 2 watchers seems unnecessary.

Let's step back and talk about what we want here.  Are you trying to provide CSS reloading for any file throughout DevTools, but then the React component reloading is specific to BrowserLoader?

If I am understanding correctly, I think it would be better to start / manage a single watcher from a central place.  Something like Loader.jsm or similar.  I am thinking it would start when on first use of any loader actions, not once per loader instance.  Would need to work out when to stop it though.

Then, from that central place, you could put the CSS reloading which seems like it could apply anyway.  The React component stuff would remain here in BrowserLoader, since that's how we use React today.

Let's talk more on IRC / Vidyo if needed.

@@ +136,5 @@
> +    else {
> +      const watchFiles = require('devtools/client/shared/file-watcher.js');
> +      const watcher = watchFiles("resource://devtools/client", onFileChanged);
> +
> +      // ??? when to kill the watcher?

The BrowserLoader instance could expose an unload / destroy method that would stop the watcher.  Of course, each user would need to call that method when their tool closes (from unload event or similar).

Or, you could watch the unload event from the window yourself, but maybe that's too sneaky...?

::: devtools/client/shared/file-watcher-worker.js
@@ +22,5 @@
> +          let info;
> +          try {
> +            info = OS.File.stat(child.path);
> +          }
> +          catch(e) {

} catch (e) {

::: devtools/client/shared/file-watcher.js
@@ +17,5 @@
> +    // `/home/james/gecko-dev/devtools/foo` by resolving a root
> +    // `resource://` path and switching it out. We don't want a local
> +    // path into the built directory, so we need to strip off the obj-*
> +    // parts of the path. This assumes Firefox is running a local build.
> +    resolvedRootURI = resolveResourceURI("resource://")

I think it's best to resolve the `baseURI` first, which is somewhere inside devtools.  This will help this feature coexist with :ochameau's add-on workflow, because it will remap resource://devtools to be outside the objdir.

@@ +22,5 @@
> +      .replace(/\/obj\-.*/, "")
> +      .replace(/^file:\/\//, "");
> +    localURI = resolvedRootURI + "/" + baseURI.replace("resource://", "");
> +  }
> +  else {

} else {

@@ +31,5 @@
> +    "resource://devtools/client/shared/file-watcher-worker.js"
> +  );
> +
> +  watchWorker.onmessage = event => {
> +    if(resolvedRootURI) {

if (

::: devtools/client/shared/vendor/moz.build
@@ +7,5 @@
>  DevToolsModules(
>      'react-dom.js',
>      'react-redux.js',
>      'react.js',
> +    'ReactProxy.js',

For consistency with other files, and also so the file matches the project's package name[1], please name this file `react-proxy.js`.

[1]: https://github.com/gaearon/react-proxy/blob/master/package.json#L2

Comment 12

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Comment on attachment 8708025 [details] [diff] [review]
> 1239018.patch
> 
> Review of attachment 8708025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/preferences/devtools.js
> @@ +16,5 @@
> >  
> >  // Disable the error console
> >  pref("devtools.errorconsole.enabled", false);
> >  
> > +// Developer prefs
> 
> Nit: Pretty ambiguous since all the tools are for developers... maybe
> "DevTools development workflow" or something.
> 
> ::: devtools/client/shared/browser-loader.js
> @@ +51,5 @@
> >      sharedGlobal: true,
> >      sandboxPrototype: window,
> >      paths: Object.assign({}, loaderOptions.paths),
> >      invisibleToDebugger: loaderOptions.invisibleToDebugger,
> > +    requireHook: (id, require) => {
> 
> Hmm, why does this name change?  Isn't `require` the correct name?

I have another patch that tweaks this options to have more consistent names, since I'm adding another hook: https://bugzilla.mozilla.org/show_bug.cgi?id=1239060

> @@ +63,5 @@
> >        }
> >  
> >        return require(uri);
> > +    },
> > +    exportsHook: (uri, exports, require) => {
> 
> Is this a loader option?  I see no references to `exportsHook`.

Yep, see the bug I linked to above

> @@ +93,5 @@
> > +    const { TargetFactory } = devtools.require("devtools/client/framework/target");
> > +    const { gDevTools } = devtools.require("resource://devtools/client/framework/gDevTools.jsm");
> > +
> > +    function onFileChanged(fileURI) {
> > +      dump('Changed: ' + fileURI + '\n');
> 
> Use " everywhere
> 
> @@ +128,5 @@
> > +    if(TargetFactory.isKnownTab(tab)) {
> > +      const target = TargetFactory.forTab(tab);
> > +      const toolbox = gDevTools.getToolbox(target);
> > +
> > +      toolbox.on("file-changed", (_, fileURI) => {
> 
> I am still not sure that the watching should be in the toolbox.  So far, the
> only code that watches "file-changed" events in the BrowserLoader, but then
> you have the entire toolbox emitting events.  If I don't open a tool that
> uses BrowserLoader, the toolbox will still be emitting events, but no one
> will listen, even though I could probably be using at least the CSS part.
> 
> Also, if it is in the toolbox, what happens with 2 toolboxes open in
> different tabs?  Spinning up 2 watchers seems unnecessary.
> 
> Let's step back and talk about what we want here.  Are you trying to provide
> CSS reloading for any file throughout DevTools, but then the React component
> reloading is specific to BrowserLoader?
> 
> If I am understanding correctly, I think it would be better to start /
> manage a single watcher from a central place.  Something like Loader.jsm or
> similar.  I am thinking it would start when on first use of any loader
> actions, not once per loader instance.  Would need to work out when to stop
> it though.
> 
> Then, from that central place, you could put the CSS reloading which seems
> like it could apply anyway.  The React component stuff would remain here in
> BrowserLoader, since that's how we use React today.
> 
> Let's talk more on IRC / Vidyo if needed.

Definitely worth talking about. I agree that we could move file watching up even more central so that multiple toolboxes could event listen to a single source.

However, we can't do CSS reloading anywhere but in BrowserLoader or the document itself. It needs access to the current `document` instance to even know how to reload it. This monkeypatches the DOM, so it inherently needs to be somewhere in the DOM (BrowserLoader will automatically do this, but we could expose a simple function for other tools that don't use it yet to enable the hook)

This is all focusing on just frontend reloading right now, so it needs to be in an environment with a `document`.

Totally agree though that we could emit the actual `file-changed` event from somewhere else. I think we'd still have the separation we have here, except BrowserLoader would load in a special module from the devtools loader and listen to that instead.

Let's talk about it more over IRC tomorrow though.

> 
> @@ +136,5 @@
> > +    else {
> > +      const watchFiles = require('devtools/client/shared/file-watcher.js');
> > +      const watcher = watchFiles("resource://devtools/client", onFileChanged);
> > +
> > +      // ??? when to kill the watcher?
> 
> The BrowserLoader instance could expose an unload / destroy method that
> would stop the watcher.  Of course, each user would need to call that method
> when their tool closes (from unload event or similar).
> 
> Or, you could watch the unload event from the window yourself, but maybe
> that's too sneaky...?

Moving the watcher into a system module that's only ever loaded once solves all of these issues, so I do like that idea.

> ::: devtools/client/shared/file-watcher-worker.js
> @@ +22,5 @@
> > +          let info;
> > +          try {
> > +            info = OS.File.stat(child.path);
> > +          }
> > +          catch(e) {
> 
> } catch (e) {
> 
> ::: devtools/client/shared/file-watcher.js
> @@ +17,5 @@
> > +    // `/home/james/gecko-dev/devtools/foo` by resolving a root
> > +    // `resource://` path and switching it out. We don't want a local
> > +    // path into the built directory, so we need to strip off the obj-*
> > +    // parts of the path. This assumes Firefox is running a local build.
> > +    resolvedRootURI = resolveResourceURI("resource://")
> 
> I think it's best to resolve the `baseURI` first, which is somewhere inside
> devtools.  This will help this feature coexist with :ochameau's add-on
> workflow, because it will remap resource://devtools to be outside the objdir.

We need to know what to strip though. A `resource://foo/bar.js` path will resolve to `file:///Users/james/gecko-dev/obj-x86_64-apple-etc/dist/Nightly.app/Contents/......./foo/bar.js`. What we actually want is `/Users/james/geck-dev/foo/bar.js`. So we need to strip the file://, but most importantly also all the `obj-*/dist/Nightly.app...` stuff, which is in the middle. I don't see how else to do that.

I added the file:// support because I thought that would help coexist with Alex's work but I haven't looked much at it.

> 
> @@ +22,5 @@
> > +      .replace(/\/obj\-.*/, "")
> > +      .replace(/^file:\/\//, "");
> > +    localURI = resolvedRootURI + "/" + baseURI.replace("resource://", "");
> > +  }
> > +  else {
> 
> } else {
> 
> @@ +31,5 @@
> > +    "resource://devtools/client/shared/file-watcher-worker.js"
> > +  );
> > +
> > +  watchWorker.onmessage = event => {
> > +    if(resolvedRootURI) {
> 
> if (
> 
> ::: devtools/client/shared/vendor/moz.build
> @@ +7,5 @@
> >  DevToolsModules(
> >      'react-dom.js',
> >      'react-redux.js',
> >      'react.js',
> > +    'ReactProxy.js',
> 
> For consistency with other files, and also so the file matches the project's
> package name[1], please name this file `react-proxy.js`.
> 
> [1]: https://github.com/gaearon/react-proxy/blob/master/package.json#L2

Yeah, true, that was just the file that got generated.

(didn't respond to all the nits, I'll clean it up)

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #12)
> > ::: devtools/client/shared/file-watcher-worker.js
> > @@ +22,5 @@
> > > +          let info;
> > > +          try {
> > > +            info = OS.File.stat(child.path);
> > > +          }
> > > +          catch(e) {
> > 
> > } catch (e) {
> > 
> > ::: devtools/client/shared/file-watcher.js
> > @@ +17,5 @@
> > > +    // `/home/james/gecko-dev/devtools/foo` by resolving a root
> > > +    // `resource://` path and switching it out. We don't want a local
> > > +    // path into the built directory, so we need to strip off the obj-*
> > > +    // parts of the path. This assumes Firefox is running a local build.
> > > +    resolvedRootURI = resolveResourceURI("resource://")
> > 
> > I think it's best to resolve the `baseURI` first, which is somewhere inside
> > devtools.  This will help this feature coexist with :ochameau's add-on
> > workflow, because it will remap resource://devtools to be outside the objdir.
> 
> We need to know what to strip though. A `resource://foo/bar.js` path will
> resolve to
> `file:///Users/james/gecko-dev/obj-x86_64-apple-etc/dist/Nightly.app/
> Contents/......./foo/bar.js`. What we actually want is
> `/Users/james/geck-dev/foo/bar.js`. So we need to strip the file://, but
> most importantly also all the `obj-*/dist/Nightly.app...` stuff, which is in
> the middle. I don't see how else to do that.
> 
> I added the file:// support because I thought that would help coexist with
> Alex's work but I haven't looked much at it.

How about:

// baseURI is "resource://devtools/client"

function watchFiles(baseURI, onFileChanged) {
  let resolvedRootURI, localURI;
 
  if (baseURI.startsWith("file://")) {
    localURI = baseURI;
  } else if (baseURI.startsWith("resource://")) {
    let basePath = "/" + baseURI.replace("resource://", "");
    resolvedRootURI = resolveResourceURI(baseURI);
      .replace(basePath, "")
      .replace(/\/obj\-.*/, "")
      .replace(/^file:\/\//, "");
    localURI = resolvedRootURI + basePath;
  } else {
    throw new Error("Must pass a resource:// or file:// URI to `watchFiles`");
  }

  ...
}

It should handle :ochameau and regular builds by transforming both:

file:///Users/james/gecko-dev/obj-x86_64-apple-etc/dist/Nightly.app/Contents/foo/bar.js
file:///Users/james/gecko-dev/foo/bar.js

to:

/Users/james/gecko-dev

as the `resolvedRootURI`.

Comment 14

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> > 
> > We need to know what to strip though. A `resource://foo/bar.js` path will
> > resolve to
> > `file:///Users/james/gecko-dev/obj-x86_64-apple-etc/dist/Nightly.app/
> > Contents/......./foo/bar.js`. What we actually want is
> > `/Users/james/geck-dev/foo/bar.js`. So we need to strip the file://, but
> > most importantly also all the `obj-*/dist/Nightly.app...` stuff, which is in
> > the middle. I don't see how else to do that.
> > 
> > I added the file:// support because I thought that would help coexist with
> > Alex's work but I haven't looked much at it.
> 
> How about:
> 
> // baseURI is "resource://devtools/client"
> 
> function watchFiles(baseURI, onFileChanged) {
>   let resolvedRootURI, localURI;
>  
>   if (baseURI.startsWith("file://")) {
>     localURI = baseURI;
>   } else if (baseURI.startsWith("resource://")) {
>     let basePath = "/" + baseURI.replace("resource://", "");
>     resolvedRootURI = resolveResourceURI(baseURI);
>       .replace(basePath, "")
>       .replace(/\/obj\-.*/, "")
>       .replace(/^file:\/\//, "");
>     localURI = resolvedRootURI + basePath;
>   } else {
>     throw new Error("Must pass a resource:// or file:// URI to
> `watchFiles`");
>   }
> 
>   ...
> }
> 
> It should handle :ochameau and regular builds by transforming both:
> 
> file:///Users/james/gecko-dev/obj-x86_64-apple-etc/dist/Nightly.app/Contents/
> foo/bar.js
> file:///Users/james/gecko-dev/foo/bar.js
> 
> to:
> 
> /Users/james/gecko-dev
> 
> as the `resolvedRootURI`.

Hm, replacing `basePath` like that might replace the wrong thing, with certain paths? It might work though.

I'm a little unclear what the requirements are; you are using file:// there, but I was assuming that if you are using file:// it is point to the straight source already and we don't even need to resolve back to a resource:// URI. At this point it's probably best to catch up on IRC tomorrow. (I should take a look at Alex's work too)

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #14)
> Hm, replacing `basePath` like that might replace the wrong thing, with
> certain paths? It might work though.

Yeah, might need a bit more thought to ensure it does the right thing.

> I'm a little unclear what the requirements are; you are using file:// there,
> but I was assuming that if you are using file:// it is point to the straight
> source already and we don't even need to resolve back to a resource:// URI.
> At this point it's probably best to catch up on IRC tomorrow. (I should take
> a look at Alex's work too)

:ochameau's add-on approach installs a chrome.manifest that overrides resource://devtools to point at your source checkout.  

So, if you resolve "resource://devtools", with a regular build you would get:

file:///Users/james/gecko-dev/obj-x86_64-apple-etc/dist/Nightly.app/Contents/.../devtools

With the add-on applied, you instead resolve to:

file:///Users/james/gecko-dev/devtools

Comment 16

[James Long (:jlongster)]

Attachment: 1239018.patch

I like this a lot better. We just fire up one file-watcher by doing it immediately when the module is loaded, and the devtools loader should cache that module so it's only ever loaded one. Doing so, we never have to worry about "stopping" the watching, and everything has access to it.

It wouldn't be hard to make file-watcher.js dynamically watch the pref so it starts/stops watching whenever the pref is flipped. That means you wouldn't have to restart.

I still also need to only include react-proxy.js if DEBUG_JS_MODULES is set. However, given those 2 things does this look good?

Comment 17

[James Long (:jlongster)]

Attachment: 1239018.patch

I went ahead and only added react-proxy.js if DEBUG_JS_MODULES is on.

This should be close to land, no? I can do the dynamic pref watching as a follow-up. I guess we still need to talk about the pref name. I guess `devtools.loader.hotreload` works.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8708523 [details] [diff] [review]
1239018.patch

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

Seems pretty close, but many linting problems still left to fix.

::: devtools/client/preferences/devtools.js
@@ +17,5 @@
>  // Disable the error console
>  pref("devtools.errorconsole.enabled", false);
>  
> +// Developer prefs
> +pref("devtools.hotreload", false);

Yes, let's make it devtools.loader.hotreload.  And a better comment like I mention in comment 11.

::: devtools/client/shared/browser-loader.js
@@ +17,5 @@
> +  obs.notifyObservers(null, "startupcache-invalidate", null);
> +}
> +
> +function hotReloadFile(window, require, loader, componentProxies, fileURI) {
> +  dump('Hot reloading: ' + fileURI + '\n');

Use " everywhere.

Turn on ESLint.

@@ +27,5 @@
> +    if (proxy) {
> +      // Remove the old module and re-require the new one; the require
> +      // hook in the loader will take care of the rest
> +      delete loader.modules[fileURI];
> +      clearCache();

Are you sure this is really needed?  It actually fails to pick up changes without it?

::: devtools/client/shared/file-watcher.js
@@ +44,5 @@
> +  watchWorker.postMessage({ path: localURI, fileRegex: /\.(js|css)$/ });
> +  return watchWorker;
> +}
> +
> +const emitter = new EventEmitter();

You could also do:

EventEmitter.decorate(module.exports);

so then the usage is:

const watcher = devtools.require("devtools/client/shared/file-watcher");
watcher.on('file-changed', ...);

Gets rid of the `emitter` key which feels a little strange to me.

Comment 19

[James Long (:jlongster)]

Attachment: 1239018.patch

Comment 20

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> Comment on attachment 8708523 [details] [diff] [review]
> 1239018.patch
> 
> Review of attachment 8708523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems pretty close, but many linting problems still left to fix.
> 
> ::: devtools/client/preferences/devtools.js
> @@ +17,5 @@
> >  // Disable the error console
> >  pref("devtools.errorconsole.enabled", false);
> >  
> > +// Developer prefs
> > +pref("devtools.hotreload", false);
> 
> Yes, let's make it devtools.loader.hotreload.  And a better comment like I
> mention in comment 11.

Done

> 
> ::: devtools/client/shared/browser-loader.js
> @@ +17,5 @@
> > +  obs.notifyObservers(null, "startupcache-invalidate", null);
> > +}
> > +
> > +function hotReloadFile(window, require, loader, componentProxies, fileURI) {
> > +  dump('Hot reloading: ' + fileURI + '\n');
> 
> Use " everywhere.
> 
> Turn on ESLint.
> 

Oops, I forgot to run it through. Yeah, fixed all of that. I know I need to automate that.

> @@ +27,5 @@
> > +    if (proxy) {
> > +      // Remove the old module and re-require the new one; the require
> > +      // hook in the loader will take care of the rest
> > +      delete loader.modules[fileURI];
> > +      clearCache();
> 
> Are you sure this is really needed?  It actually fails to pick up changes
> without it?

Yep. It's common when dealing with loaders to hit this; it just is picked up from the browser cache if you don't do this. I was not seeing updates until I remembered that I had to do this.

> 
> ::: devtools/client/shared/file-watcher.js
> @@ +44,5 @@
> > +  watchWorker.postMessage({ path: localURI, fileRegex: /\.(js|css)$/ });
> > +  return watchWorker;
> > +}
> > +
> > +const emitter = new EventEmitter();
> 
> You could also do:
> 
> EventEmitter.decorate(module.exports);
> 
> so then the usage is:
> 
> const watcher = devtools.require("devtools/client/shared/file-watcher");
> watcher.on('file-changed', ...);
> 
> Gets rid of the `emitter` key which feels a little strange to me.

Yeah, I initially did `module.exports = new EventEmitter()` actually, evidently you can't do that though because exported objects are frozen, so it can't change any internal state. I just realized that `decorate` creates an internal emitter object though, so I think that would work. (I've always found the `decorate` API a little weird)

Just updated it, and yep it works!

Thanks for the quick review!

Comment 21

[James Long (:jlongster)]

Attachment: 1239018.patch

Main thing this is blocked on now is bug 1239060. Hopefully we can land all of this next week though.

Comment 22

[James Long (:jlongster)]

Attachment: 1239018.patch

Ryan, think you could do an official review now and give it a r+? I addressed your previous comments, and I think it's a pretty good place to start (and harmless since pref is default off)

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8709463 [details] [diff] [review]
1239018.patch

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

Thanks for working on this!  Add r=jryans to commit.

::: devtools/client/shared/browser-loader.js
@@ +19,5 @@
>  
> +function clearCache() {
> +  const obs = Cc["@mozilla.org/observer-service;1"]
> +        .getService(Ci.nsIObserverService);
> +  obs.notifyObservers(null, "startupcache-invalidate", null);

Use `Services.obs`

Comment 24

[James Long (:jlongster)]

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

Comment 25

[James Long (:jlongster)]

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

Comment 26

[James Long (:jlongster)]

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

Comment 27

[James Long (:jlongster)]

Attachment: 1239018.patch

Tweaked patch

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a11b6d294b85

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a11b6d294b85