1237253 - Support define in BrowserLoader

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

We should remove requirejs and use standard package manager like e.g. webpack to produce a single bundle that is loaded using standard script tag.

The bundling can be integrated with the current build system or done manually.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@James, how far we are from having webpack built-in devtools?
Or what process we could utilize here?

Honza

Comment 2

[:Gijs (he/him)]

(In reply to Jan Honza Odvarko [:Honza] from bug 1232548 comment #14)
> (In reply to :Gijs Kruitbosch from bug 1232548 comment #13)
> > Right, in which case, can you investigate if/why the viewer leaks without
> > this patch?
> I believe that leaks are related to 'promiseTabLoaded' API in the test. This
> function waits till 'load' event is fired from the content, but JSON Viewer
> isn't done yet and => the window leaks.
> 
> There is an event "JSONViewInitialized" that is fired from the content when
> JSON Viewer is fully initialized:
> https://github.com/mozilla/gecko-dev/blob/master/devtools/client/jsonview/
> json-viewer.js#L94
> 
> Utilized in tests already:
> https://github.com/mozilla/gecko-dev/blob/master/devtools/client/jsonview/
> test/doc_frame_script.js#L31

But why do we leak in this case? It sounds like this could also happen in "real" usage if we unloaded the document at the "wrong" time, ie between the firing of the load event and when the modules are loaded. That should not be the case, ie we should not leak in what is otherwise a "normal" interaction with the browser.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to :Gijs Kruitbosch from comment #2)
> But why do we leak in this case?
Yes, this is puzzling me too. Could it be that the test finishes before the window is loaded and the leak detector evaluates it as a leak? It doesn't have to be real leak.

Honza

Comment 4

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

(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> @James, how far we are from having webpack built-in devtools?
> Or what process we could utilize here?

I would say it's still unknown if or when we might add a build step like webpack.  We'll need more discussion on the team before possibly doing something like that.  Personally, I would like to avoid a build step if we can, but it's a complex problem.

Instead, you could use :jlong's BrowserLoader for now:

https://dxr.mozilla.org/mozilla-central/search?q=BrowserLoader&case=true&=mozilla-central&redirect=true

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> (In reply to Jan Honza Odvarko [:Honza] from comment #1)
> > @James, how far we are from having webpack built-in devtools?
> > Or what process we could utilize here?
> 
> I would say it's still unknown if or when we might add a build step like
> webpack.  We'll need more discussion on the team before possibly doing
> something like that.  Personally, I would like to avoid a build step if we
> can, but it's a complex problem
Agree, avoiding a build step is good.

> Instead, you could use :jlong's BrowserLoader for now:
I can't use it. BrowserLoader needs chrome scope, but JSON Viewer app runs within content scope.

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1237253-1.patch

James, I am attaching a patch that adds support for `define()` (AMD modules). This way we can load modules using AMD just like any other module in our code base.

Some comments:

1) It's rather a workaround so, we don't have to add new build step (webpack) now. It looks like this needs more discussion.
2) The loader allows reusing modules from JSONViewer. JSON Viewer is using AMD module syntax + RJS and it's running entirely in content scope.

To summarize, BrowserLoader can be used to load AMD modules:

define(function(require, exports, module) {
  /* Module definition */
});

Honza

Comment 7

[James Long (:jlongster)]

Comment on attachment 8706397 [details] [diff] [review]
bug1237253-1.patch

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

r- just because I want to support jryan and other's work to get away from magic paths. I'd like to have node_modules-style resolution, but that's a separate discussion. I'll get jryans to look at this and see if I'm wrong.

::: devtools/client/shared/browser-loader.js
@@ +13,5 @@
> +const PATH_MAPPING = {
> +  "react": "resource://devtools/client/shared/vendor/react",
> +  "react-dom": "resource://devtools/client/shared/vendor/react-dom"
> +};
> +

We're strongly discouraging magic paths, so I don't think this is going to work. We want all paths to map directly to the filesystem.

We've talked about supporting a node_modules-style lookup, and that way it would still be a clear resolution strategy. But jryans has put a lot of effort in to get away magic paths for now.

It's probably easier right now to add `devtools/client/shared/vendor/react` to your require.js config. jryans can probably explain better why we want to be careful going down this route.

@@ +62,5 @@
>        return require(uri);
> +    },
> +    globals: {
> +      define: function(callback) {
> +        callback(this.require, this.exports, this.module);

How does this work? Where is `this` coming from? Won't it be `null` in strict mode?

I'm fine with this strategy, btw. I'm sad we require two different loaders, but it works for now.

Comment 8

[James Long (:jlongster)]

Ryan, mind looking at the above patch? We want to avoid magic paths for now, right?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to James Long (:jlongster) from comment #7)
> It's probably easier right now to add `devtools/client/shared/vendor/react`
Sure, this works for me.

> How does this work? Where is `this` coming from?
`this` points to the current module (executing `define`) and it's used to access `require`, `exports` and `module` globals. Or, is there a better way how to get those vars and pass them into `define`?

> Won't it be `null` in strict mode?
All files are using "use strict" or do you have `javascript.options.strict` pref in mind?

Honza

Comment 10

[James Long (:jlongster)]

(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> (In reply to James Long (:jlongster) from comment #7)
> > It's probably easier right now to add `devtools/client/shared/vendor/react`
> Sure, this works for me.
> 
> > How does this work? Where is `this` coming from?
> `this` points to the current module (executing `define`) and it's used to
> access `require`, `exports` and `module` globals. Or, is there a better way
> how to get those vars and pass them into `define`?
> 
> > Won't it be `null` in strict mode?
> All files are using "use strict" or do you have `javascript.options.strict`
> pref in mind?

I meant normal "use strict" mode. You are using `this` inside the the `define` function, but why is `this` the module? Is that because `define` is being called in the global scope? In strict mode, I thought `this` was undefined inside functions.

I just noticed that browser-loader.js does not have "use strict"; at the top. If you do that, does it change anything?

Comment 11

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

Comment on attachment 8706397 [details] [diff] [review]
bug1237253-1.patch

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

I am okay with the overall idea, just no magic paths please.

::: devtools/client/shared/browser-loader.js
@@ +13,5 @@
> +const PATH_MAPPING = {
> +  "react": "resource://devtools/client/shared/vendor/react",
> +  "react-dom": "resource://devtools/client/shared/vendor/react-dom"
> +};
> +

I agree, no more magic paths if we can help it.  We went through lots of trouble lately to move away from them.

Instead you should require something like "devtools/client/shared/vendor/react" as the module ID.  The module ID should match the path in the tree exactly.

This makes it much easier to trace the connection from module ID to file on disk.  We don't have to have any magic tables to look them up in, and we can make things like reload work far more easily this way.

@@ +61,5 @@
>  
>        return require(uri);
> +    },
> +    globals: {
> +      define: function(callback) {

Please add a comment about this use case (allows reusing AMD modules) to clarify why this is here.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1237253-2.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Comment on attachment 8706397 [details] [diff] [review]
> bug1237253-1.patch
> 
> Review of attachment 8706397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am okay with the overall idea, just no magic paths please.
Agreed

> Instead you should require something like
> "devtools/client/shared/vendor/react" as the module ID.  The module ID
> should match the path in the tree exactly.
Done

> > +    },
> > +    globals: {
> > +      define: function(callback) {
> 
> Please add a comment about this use case (allows reusing AMD modules) to
> clarify why this is here.
Done 

Also, I fixed one forgotten 'react' magic-path from react-dom.js

(In reply to James Long (:jlongster) from comment #10)
> I just noticed that browser-loader.js does not have "use strict"; at the
> top. If you do that, does it change anything?
browser-loader updated, now it has a license comment + "use strict"; at the top.
All still work for me.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6048d167851f


Honza

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am also updating the bug title so, it fits more what we are doing here.
Honza

Comment 14

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

Comment on attachment 8707408 [details] [diff] [review]
bug1237253-2.patch

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

Update commit message to add me as a reviewer.

::: devtools/client/jsonview/viewer-config.js
@@ +22,5 @@
>   */
>  require.config({
>    baseUrl: ".",
>    paths: {
> +    "devtools/client/shared/vendor/react": [

You can simplify this paths mapping to:

"devtools": "resource://devtools"

There is no `react-dev` anymore, so that special case is not needed.

::: devtools/client/shared/browser-loader.js
@@ +8,5 @@
>  const loaders = Cu.import("resource://gre/modules/commonjs/toolkit/loader.js", {});
>  const { devtools, DevToolsLoader } = Cu.import("resource://devtools/shared/Loader.jsm", {});
>  const { joinURI } = devtools.require("devtools/shared/path");
>  const BROWSER_BASED_DIRS = [
> +  "resource://devtools/client/jsonview",

Out of curiosity, what parts of the JSON viewer are you wanting to reuse?

If they are going to be shared with a different tool, does it make sense to move them over to the shared directory?

Comment 15

[James Long (:jlongster)]

react-dev.js is coming back in bug 1224765. We really need to get the production version working again, and I'm going to land this this week, so probably worth keeping that check in.

Comment 16

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

(In reply to James Long (:jlongster) from comment #15)
> react-dev.js is coming back in bug 1224765. We really need to get the
> production version working again, and I'm going to land this this week, so
> probably worth keeping that check in.

Ah, fair enough.  I thought you were going to do it all in one file and check AppConstants within React.  I don't feel strongly either way.

Comment 17

[James Long (:jlongster)]

I thought about that, but it was more of a pain to patch React's code and we still had the problem of it not including certain testing utilities in prod mode. What we were doing before worked, just had a couple bugs and we didn't have time to fix them before.

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1237253-3.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)

> Update commit message to add me as a reviewer.
Done

> You can simplify this paths mapping to:
> 
> "devtools": "resource://devtools"
Since, react-dev is coming back I am keeping this as it is.

> Out of curiosity, what parts of the JSON viewer are you wanting to reuse?
The entire 'devtools/jsonviewer/components/reps' directory. I.e. the set of components that allow to render JS objects and primitives. It'll be reused by the XHR inspection feature (in the Console panel) since it also displays JS data (e.g. parsed JSON HTTP responses as an expandable tree).

> If they are going to be shared with a different tool, does it make sense to
> move them over to the shared directory?
Definitely.

I am thinking about moving the 'reps' dir into 'devtools/client/shared/components/reps'
Does that make sense? James?

Thanks for the review Ryan!

Honza

Comment 19

[James Long (:jlongster)]

(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> I am thinking about moving the 'reps' dir into
> 'devtools/client/shared/components/reps'
> Does that make sense? James?

I'm not sure. There's some tension in that you want to use these components in various places, but we also *just* started figuring out the guidelines for components inside that directory. How would you use it in the JSON Viewer? We can't have AMD modules in there, that's just way too confusing.

The AMD modules seem to be the main blocker here. I'm still convinced that you should use webpack to build the JSON Viewer...

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to James Long (:jlongster) from comment #19)
> The AMD modules seem to be the main blocker here. I'm still convinced that
> you should use webpack to build the JSON Viewer...
It's fine for me to use it so, I've filled Bug 1240494 for it.
We can continue discussion there.

Honza

Comment 21

[Pulsebot]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fb425263835

Comment 23

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected