Support define in BrowserLoader

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: Honza, Assigned: Honza)

Tracking

Trunk
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
@James, how far we are from having webpack built-in devtools?
Or what process we could utilize here?

Honza
Flags: needinfo?(jlong)
(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.
(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
(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
(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
Posted patch bug1237253-1.patch (obsolete) — Splinter Review
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
Assignee: nobody → odvarko
Attachment #8706397 - Flags: review?(jlong)
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.
Attachment #8706397 - Flags: review?(jlong) → review-
Ryan, mind looking at the above patch? We want to avoid magic paths for now, right?
Flags: needinfo?(jlong) → needinfo?(jryans)
(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
(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 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.
Attachment #8706397 - Flags: review-
Flags: needinfo?(jryans)
Posted patch bug1237253-2.patch (obsolete) — Splinter Review
(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
Attachment #8706397 - Attachment is obsolete: true
Attachment #8707408 - Flags: review?(jryans)
I am also updating the bug title so, it fits more what we are doing here.
Honza
Summary: JSON View: remove requirejs → Support define in BrowserLoader
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?
Attachment #8707408 - Flags: review?(jryans) → review+
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.
(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.
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.
(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
Attachment #8707408 - Attachment is obsolete: true
Flags: needinfo?(jlong)
Attachment #8708311 - Flags: review+
(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...
Flags: needinfo?(jlong)
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4fb425263835
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.