1216657 - Use react-dev based off of build flag, and work in all loaders

Status: NEW

(DevTools :: General, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

PropTypes are only enforced when using the dev build. We should do this in testing to ensure our model validations are working in tests.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1216657-react-path.patch

r? jryans for loader changes, r? janx for about:debugging changes

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8676572 [details] [diff] [review]
1216657-react-path.patch

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

::: devtools/shared/Loader.jsm
@@ +102,5 @@
>          // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
>          // Allow access to xpcshell test items from the loader.
> +        "xpcshell-test": "resource://test",
> +        // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
> +        "react": AppConstants.DEBUG_JS_MODULES ?

This should probably also return react-dev when normal DEBUG.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

It's my understanding that DEBUG_JS_MODULES is the JS version of `DEBUG`, and if we do things if either is set on, then that defeats the purpose of it right?

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

That is, DEBUG_JS without DEBUG makes sense otherwise everything's super slow, but DEBUG without DEBUG_JS doesn't make sense to me

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8676572 [details] [diff] [review]
1216657-react-path.patch

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

Doesn't break about:debugging so fine by me.

Comment 7

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

Comment on attachment 8676572 [details] [diff] [review]
1216657-react-path.patch

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

I'd like to keep the "exact path" module ID "devtools/client/shared/vendor/react" until we have a real solution for loading vendor modules similar to Node, etc.  Once such a thing does exist, we can rejoice and use short names if we want.  I'll keep thinking about ideas for this in bug 1201710.

::: devtools/shared/Loader.jsm
@@ +102,5 @@
>          // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
>          // Allow access to xpcshell test items from the loader.
> +        "xpcshell-test": "resource://test",
> +        // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠
> +        "react": AppConstants.DEBUG_JS_MODULES ?

I don't feel strongly, but it seems nice to let the flags be independent, so you have more control of what's happening.

We can always tweak which flags are consulted, so I am not too worried.

@@ +150,5 @@
>      let acornURI = this.fileURI(OS.Path.join(sharedDir, "acorn"));
>      let acornWalkURI = OS.Path.join(acornURI, "walk.js");
>      let sourceMapURI = this.fileURI(OS.Path.join(sharedDir,
>                                                   "sourcemap", "source-map.js"));
> +    let reactURI = this.fileURI(OS.path.join(devtoolsDir, "shared", "vendor",

The "client" path segment is missing here.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8676572 [details] [diff] [review]
> 1216657-react-path.patch
> 
> Review of attachment 8676572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to keep the "exact path" module ID
> "devtools/client/shared/vendor/react" until we have a real solution for
> loading vendor modules similar to Node, etc.  Once such a thing does exist,
> we can rejoice and use short names if we want.  I'll keep thinking about
> ideas for this in bug 1201710.

So just have "devtools/client/shared/vendor/react" either map to prod or dev version? 

> @@ +150,5 @@
> >      let acornURI = this.fileURI(OS.Path.join(sharedDir, "acorn"));
> >      let acornWalkURI = OS.Path.join(acornURI, "walk.js");
> >      let sourceMapURI = this.fileURI(OS.Path.join(sharedDir,
> >                                                   "sourcemap", "source-map.js"));
> > +    let reactURI = this.fileURI(OS.path.join(devtoolsDir, "shared", "vendor",
> 
> The "client" path segment is missing here.

Huh, I guess I was only testing (and on try too?) the other branch. I don't quite get the different "providers" here.

Comment 9

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> > Comment on attachment 8676572 [details] [diff] [review]
> > 1216657-react-path.patch
> > 
> > Review of attachment 8676572 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'd like to keep the "exact path" module ID
> > "devtools/client/shared/vendor/react" until we have a real solution for
> > loading vendor modules similar to Node, etc.  Once such a thing does exist,
> > we can rejoice and use short names if we want.  I'll keep thinking about
> > ideas for this in bug 1201710.
> 
> So just have "devtools/client/shared/vendor/react" either map to prod or dev
> version? 

Right.  That's how browser loader handled the situation before.

> > @@ +150,5 @@
> > >      let acornURI = this.fileURI(OS.Path.join(sharedDir, "acorn"));
> > >      let acornWalkURI = OS.Path.join(acornURI, "walk.js");
> > >      let sourceMapURI = this.fileURI(OS.Path.join(sharedDir,
> > >                                                   "sourcemap", "source-map.js"));
> > > +    let reactURI = this.fileURI(OS.path.join(devtoolsDir, "shared", "vendor",
> > 
> > The "client" path segment is missing here.
> 
> Huh, I guess I was only testing (and on try too?) the other branch. I don't
> quite get the different "providers" here.

There is currently no real testing of the SrcdirProvider, aside from ensuring it has the correct number of path lines in it (but they can still be invalid, probably a test should check that at some point).  It's only used if you set a "srcdir" path to load files directly from your source tree, instead of from a fully packaged Firefox.  This is not a default setup, so currently you only go down this path if you went out of your way to turn it on.  The idea is you can use this plus the DevTools reload shortcut :ochameau added (Cmd-Opt-R) to see changes faster.

There are still a few sharp edges with it, so I believe it has no current users.  We plan to make some noise on the list about it soon though after some more polishing.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1216657-react-path.patch

This got much simpler.

Jan, no changes in aboutdebugging anymore.

Comment 11

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

Comment on attachment 8677563 [details] [diff] [review]
1216657-react-path.patch

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

Seems reasonable, but I think browser loader's check can still be removed.

::: devtools/shared/Loader.jsm
@@ +151,5 @@
>      let acornWalkURI = OS.Path.join(acornURI, "walk.js");
>      let sourceMapURI = this.fileURI(OS.Path.join(sharedDir,
>                                                   "sourcemap", "source-map.js"));
> +    let reactURI = this.fileURI(OS.path.join(devtoolsDir, "client", "shared", "vendor",
> +                   AppConstants.DEBUG_JS_MODULES ? "react-dev.js" : "react"));

I believe this should be "react.js" here, not just "react".

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Going in bug 960776 is fixing the browser loader to use react/react-dev correctly (not pointing at right address right now), with a note saying that this bug should get rid of it and move it to the Loader proper. This is needed for TestUtils, so once that lands, we'll atleast have a test requiring it to prevent regressions