Closed Bug 1181646 Opened 5 years ago Closed 5 years ago

Add React as shared library

Categories

(DevTools :: Debugger, defect)

40 Branch
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: jlong, Assigned: jlong)

References

Details

Attachments

(1 file, 3 obsolete files)

Let's go ahead and add React in a single place that anything related to devtools can use. bug 1132203 already needs it for the JSON response viewer (an experimental feature that is going to land).
Blocks: 1132203
Attached patch 1181646.patch (obsolete) — Splinter Review
This adds the latest React (0.13.3) to a shared directory that we can all use. Honza already needs this for bug 1132203, which is going to land soon but be only be pref-ed on in Dev Edition. And I'm going to need it pretty soon for the debugger after the first pass of my refactoring lands.

This adds both the prod and dev versions, but only builds in the dev version if you are building it locally (detected with MOZ_BRANDING_DIRECTORY="browser/branding/unofficial"). 

Right now it's up to the user to include the right one, but the BrowserLoader that I made for my debugger refactoring will automatically handle this correctly. Honza is using require.js so he can configure it accordingly, and I'm hoping eventually we'll all just use BrowserLoader (a perfect use-case for it because React needs to load in the window scope. 0.14 might make it easier to pass a window in)
Attachment #8631177 - Flags: review?(nfitzgerald)
Assignee: nobody → jlong
Attachment #8631177 - Flags: review?(nfitzgerald) → review+
Please request build peer review for this change, especially since this alters the main moz.build for toolkit.
Flags: needinfo?(jlong)
Also, why not put the files at browser/devtools/shared instead of browser/devtools/shared/browser, which is a new directory here?

shared already contains things like d3.js, so I'm not sure what the extra "browser" is meant to suggest.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Also, why not put the files at browser/devtools/shared instead of
> browser/devtools/shared/browser, which is a new directory here?
> 
> shared already contains things like d3.js, so I'm not sure what the extra
> "browser" is meant to suggest.

Things in "browser" are meant to be included in a browser environment. They require a window. Looks like d3 is this way, so it could be moved here. You could never load d3 as a module, you could only load it as a script tag. Separating this into a separate folder makes that clear.

The other big benefit is for my BrowserLoader. We found an awesome solution for loading browser files as modules: create a loader that *only* loads files needed for my window instance, and delegate everything else to the devtools loader. You have modules now, but in a normal browser environment, and all the system modules are still loaded and cached with the devtools loader. Here's the code: https://gist.github.com/jlongster/0fc1aaa1f7325ac35dc0

With an explicit path to the shared browser modules, I can also easily check it and load anything under there with the browser loader as well, which will load it within the window instance: https://gist.github.com/jlongster/0fc1aaa1f7325ac35dc0#file-browser-loader-js-L51

Otherwise I'd have to whitelist specific modules... which I could do.
It's not a huge deal if you want me to just whitelist modules that should be loaded in the browser. Would be easy to change in the future if we change our minds.
Flags: needinfo?(jlong)
Please don't forget to request build review (as I mentioned in comment 2), that's more important to me than my nits below.

(In reply to James Long (:jlongster) from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > Also, why not put the files at browser/devtools/shared instead of
> > browser/devtools/shared/browser, which is a new directory here?
> > 
> > shared already contains things like d3.js, so I'm not sure what the extra
> > "browser" is meant to suggest.
> 
> Things in "browser" are meant to be included in a browser environment. They
> require a window. Looks like d3 is this way, so it could be moved here. You
> could never load d3 as a module, you could only load it as a script tag.
> Separating this into a separate folder makes that clear.

Okay, I follow what you are trying to convey, and I think the concept is good. :)

My main nit here I guess is that the word "browser" is too overloaded so as to be almost devoid of meaning, so it doesn't immediately suggest what you are going for.  Are they files meant to be shipped with the browser?  (Yes.) One assumes so, since they are already under /browser.  Are they files about browsing?  (No.)  Are they files for managing child browsers?  (No.)

Could we use "content" as the directory name (browser/devtools/shared/content)?  This is a common name in the tree for this purpose.  While DevTools does not follow the convention very well, /browser/devtools/webide is one example: there is "content" for HTML and JS loaded in window environment / via script tags, and there is "modules" for things loaded via a CommonJS (non-window) loader.

There are many other examples of the "content" name outside devtools, like /browser/base/content, /b2g/chrome/content, etc.

> The other big benefit is for my BrowserLoader. We found an awesome solution
> for loading browser files as modules: create a loader that *only* loads
> files needed for my window instance, and delegate everything else to the
> devtools loader. You have modules now, but in a normal browser environment,
> and all the system modules are still loaded and cached with the devtools
> loader. Here's the code:
> https://gist.github.com/jlongster/0fc1aaa1f7325ac35dc0

Again, I like this concept, it seems nice.

But as far as the name, could we call this thing ContentLoader?  It doesn't load the whole browser, so BrowserLoader is unclear to me.  Also, it would be great if the loader's ID said "devtools" somewhere for clarity, so maybe "devtools-content-loader".
Flags: needinfo?(jlong)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Please don't forget to request build review (as I mentioned in comment 2),
> that's more important to me than my nits below.

How do I do that? I don't see a flag for it. I will definitely make sure to do that.

> 
> My main nit here I guess is that the word "browser" is too overloaded so as
> to be almost devoid of meaning, so it doesn't immediately suggest what you
> are going for.  Are they files meant to be shipped with the browser?  (Yes.)
> One assumes so, since they are already under /browser.  Are they files about
> browsing?  (No.)  Are they files for managing child browsers?  (No.)

> 
> Could we use "content" as the directory name
> (browser/devtools/shared/content)? 

Sure! Yeah browser is way too overloaded. I'm fine calling this anything, as long as it's available. I'd be fine with `ContentWithBrowserAPIsLoader`. But just `Content` works too.

> Also, it would
> be great if the loader's ID said "devtools" somewhere for clarity, so maybe
> "devtools-content-loader".

Will do! Thanks.
Flags: needinfo?(jlong)
(In reply to James Long (:jlongster) from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> > Please don't forget to request build review (as I mentioned in comment 2),
> > that's more important to me than my nits below.
> 
> How do I do that? I don't see a flag for it. I will definitely make sure to
> do that.

No special flag, just request an additional review from a peer of the Build Config module[1].

[1]: https://wiki.mozilla.org/Modules/All#Build_Config
Attached patch 1181646.patch (obsolete) — Splinter Review
renamed "browser" folder to "content"
Attachment #8631177 - Attachment is obsolete: true
Comment on attachment 8631726 [details] [diff] [review]
1181646.patch

Hey Ted, do you mind taking a look at this patch? I had to expose the MOZ_BRANDING_DIRECTORY to AppConstants, and needed to change toolkit/modules/moz.build for that, and jryans asked to get an additional review from someone on the build team.
Attachment #8631726 - Flags: review?(ted)
Attached patch 1181646.patch (obsolete) — Splinter Review
needed to fix a path issue
Attachment #8631726 - Attachment is obsolete: true
Attachment #8631726 - Flags: review?(ted)
Attachment #8631769 - Flags: review?(ted)
Comment on attachment 8631769 [details] [diff] [review]
1181646.patch

Redirecting to :gps, Ted's queue seems quite long.
Attachment #8631769 - Flags: review?(ted) → review?(gps)
We need a license review on this. I can already tell you that the PATENTS file referenced by the license boilerplate will almost certainly need reviewed and included in the tree.

Also, React historically had a very poorly worded license. Some very large companies were refusing to use it until Facebook changed the language. I /think/ the current version is acceptable to others now. But we should still look at it.
Flags: needinfo?(gerv)
(In reply to Gregory Szorc [:gps] from comment #13)
> We need a license review on this. I can already tell you that the PATENTS
> file referenced by the license boilerplate will almost certainly need
> reviewed and included in the tree.
> 
> Also, React historically had a very poorly worded license. Some very large
> companies were refusing to use it until Facebook changed the language. I
> /think/ the current version is acceptable to others now. But we should still
> look at it.

Note that react is already included in the tree for hello/loop.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #14)
> (In reply to Gregory Szorc [:gps] from comment #13)
> > We need a license review on this. I can already tell you that the PATENTS
> > file referenced by the license boilerplate will almost certainly need
> > reviewed and included in the tree.
> > 
> > Also, React historically had a very poorly worded license. Some very large
> > companies were refusing to use it until Facebook changed the language. I
> > /think/ the current version is acceptable to others now. But we should still
> > look at it.
> 
> Note that react is already included in the tree for hello/loop.

Yep, I believe we've already gone through this, the license is fine. The patents thing was overblown, but they changed it anyway. I'll see if one of the Hello people can comment on this.
Also note that the license is just a BSD license, without any mention of patents: https://github.com/facebook/react/blob/master/LICENSE
I need to land this asap, can we resolve this?
Comment on attachment 8631769 [details] [diff] [review]
1181646.patch

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

I'm not keen on React files being checked in to browser/devtools/shared. Surely there will be a non-devtools use of React in the near future (or perhaps Loop today already qualifies) and they'll have to do the work of moving React elsewhere. Let's just put it somewhere shared from day 0.

If I had a time machine, I'd put all third party code in a third_party/ directory at the root of the source tree. But I don't and creating a new root directory requires approval from Brendan and feels like it would draw unwanted debate. So how about toolkit/third_party/react?

::: browser/devtools/shared/content/react-dev.js
@@ +7,5 @@
> + * All rights reserved.
> + *
> + * This source code is licensed under the BSD-style license found in the
> + * LICENSE file in the root directory of this source tree. An additional grant
> + * of patent rights can be found in the PATENTS file in the same directory.

I don't feel comfortable granting r+ without this PATENTS file checked in to version control. I insist on being overridden by Gerv or someone else who understands licenses better than me.
(In reply to Gregory Szorc [:gps] from comment #18)
> Comment on attachment 8631769 [details] [diff] [review]
> 1181646.patch
> 
> Review of attachment 8631769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not keen on React files being checked in to browser/devtools/shared.
> Surely there will be a non-devtools use of React in the near future (or
> perhaps Loop today already qualifies) and they'll have to do the work of
> moving React elsewhere. Let's just put it somewhere shared from day 0.

The problem is that this creates a really strong coupling between projects about React versions. If we actually do have multiple large projects depend on this file, I can't imagine the work it would take to coordinate an upgrade of React, which would require changes across all projects. I do not want devtools to be held back (or potentially even stuck) on old versions of React. That's not acceptable.

The only benefit is saving space, right? The production version of React is about 100KB, about the size of an image. I don't think it's work this huge coupling of projects to save a tiny bit of space. Hello is already stuck on 0.12 of React until they have time to upgrade.

It may that various smaller services like Hello could share a React library, if smaller things are going to be built over time, but I'd really like devtools to have its own version of React. It's already going to be complicated enough maintaining our own code.

> ::: browser/devtools/shared/content/react-dev.js
> @@ +7,5 @@
> > + * All rights reserved.
> > + *
> > + * This source code is licensed under the BSD-style license found in the
> > + * LICENSE file in the root directory of this source tree. An additional grant
> > + * of patent rights can be found in the PATENTS file in the same directory.
> 
> I don't feel comfortable granting r+ without this PATENTS file checked in to
> version control. I insist on being overridden by Gerv or someone else who
> understands licenses better than me.

Sure, although remember React is already in our tree, and if we actually do find a problem we're going to have to remove Hello. I'm pretty sure this has already been looked at.
`hg log -f browser/components/loop/content/shared/libs/react-0.12.2.js` says 56ba52f28300 / bug 1033841 (checked in July 2014) introduced React into the tree. Looking at p1(56ba52f28300) (3f60457cbb7b), there appear to be no React files in the tree before that commit.

Looking at that bug, comment #1 makes a reference to licensing. However, there was no subsequent discussion on the subject. I can only assume React was checked in without proper licensing review. This is, uh, slightly problematic because React's contentious patents clause wasn't removed until April 10, 2015 (https://github.com/facebook/react/commit/b8ba8c83f318b84e42933f6928f231dc0918f864, which appears to fall after the 0.13.1 release).

While it's possible React's licensing was looked at post landing, I see no evidence of it. While React does appear to be using a BSD license, the boilerplate in the file requested for review makes reference to a PATENTS file. In my mind, this makes the license a variation of a standard OSS license and needs to be looked at by someone who understands these things.

I hate to put my foot down on this. And I'm confident Gerv or someone will say things are OK in the end since React's updated patents clause is apparently acceptable now. But I'm not comfortable giving a rubber stamp on licensing foo since I'm not formally versed in the subject and don't fully understand the legal requirements and consequences. I know enough about licensing that I know it is important to ask someone who understands these things for a formal sign-off. So, until there is evidence this has licensing approval, I'm forced to put my foot down and withhold granting review. Sorry :/
On PTO until Wednesday - can't look at it until then. Sorry :-(

Gerv
:gps, while we're waiting on the licensing front, I was curious what you thought of the build changes in the patch which make use of MOZ_BRANDING_DIRECTORY to discriminate between local development vs. all other builds.

Does this seem okay?
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #20)
> 
> I hate to put my foot down on this. And I'm confident Gerv or someone will
> say things are OK in the end since React's updated patents clause is
> apparently acceptable now. But I'm not comfortable giving a rubber stamp on
> licensing foo since I'm not formally versed in the subject and don't fully
> understand the legal requirements and consequences. I know enough about
> licensing that I know it is important to ask someone who understands these
> things for a formal sign-off. So, until there is evidence this has licensing
> approval, I'm forced to put my foot down and withhold granting review. Sorry
> :/

Alright, thanks for trying to track down if it had been looked into already. Sorry if I applied undue pressure, these things are important so it's ok. I was hoping to land a bunch of small bugs like this before I went on vacation, but I can wait. I respect wanting to do things right :)


(In reply to Gervase Markham [:gerv] from comment #21)
> On PTO until Wednesday - can't look at it until then. Sorry :-(
> 
> Gerv

No worries, sorry for the ping! I'm on vacation next week anyway but I'll be checking bugmail.
Comment on attachment 8631769 [details] [diff] [review]
1181646.patch

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

::: browser/devtools/shared/moz.build
@@ +53,5 @@
> +
> +if CONFIG['MOZ_BRANDING_DIRECTORY'] == 'browser/branding/unofficial':
> +    EXTRA_JS_MODULES.devtools.shared.content += [
> +        'content/react-dev.js'
> +    ]

I assume you are adding MOZ_BRANDING_DIRECTORY to AppConstants.jsm so you can do some kind of conditional import from JavaScript? e.g.

  if (MOZ_BRANDING_DIRECTORY == 'browser/branding/unofficial') {
     import "react-dev.js"
  } else {
     import "react.js"
  }

If so, I'm not too keen on this solution. I would rather there be a single react.js in the package and the version is whatever the build configuration is. Of course, if you actually need to load both variations, then we'll need both in the package. Do you?

I'm also not keen on using MOZ_BRANDING_DIRECTORY as the thing we key off of. It works, but it isn't the proper abstraction layer. We really want a DEBUGGABLE_JS_MODULES flag or something. It's not that difficult to add one to configure.in. AC_SUBST makes a variable available to moz.build. Before you do that, it might be a good idea to see if we have an existing variable/flag we can repurpose.
Attachment #8631769 - Flags: review?(gps)
Flags: needinfo?(gps)
Depends on: 1185704
Regarding the licensing issues. We landed React in Hello with version 0.10.0, as bug 1033841 shows (the hg history is based on the filename, but it doesn't show the renames). This also had licensing approval (bug 1033784).

We then missed the change of license in the 0.11.2 -> 0.12.2 upgrade.

Rather than try to go through the re-approval process here, I've split this off to bug 1185704 with a fuller explanation and we'll deal with anything related to licensing that we need to there.
Flags: needinfo?(gerv)
(In reply to Gregory Szorc [:gps] from comment #24)
> If so, I'm not too keen on this solution. I would rather there be a single
> react.js in the package and the version is whatever the build configuration
> is. Of course, if you actually need to load both variations, then we'll need
> both in the package. Do you?

Hello does this in jar.mn and keys off MOZ_DEBUG:

http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/jar.mn

Since the dev version of react is slower & does more checks, this seemed like a reasonable thing to do for us.
(In reply to Gregory Szorc [:gps] from comment #24)
> Comment on attachment 8631769 [details] [diff] [review]
> 1181646.patch
> 
> Review of attachment 8631769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/moz.build
> @@ +53,5 @@
> > +
> > +if CONFIG['MOZ_BRANDING_DIRECTORY'] == 'browser/branding/unofficial':
> > +    EXTRA_JS_MODULES.devtools.shared.content += [
> > +        'content/react-dev.js'
> > +    ]
> 
> I assume you are adding MOZ_BRANDING_DIRECTORY to AppConstants.jsm so you
> can do some kind of conditional import from JavaScript? e.g.
> 
>   if (MOZ_BRANDING_DIRECTORY == 'browser/branding/unofficial') {
>      import "react-dev.js"
>   } else {
>      import "react.js"
>   }
> 
> If so, I'm not too keen on this solution. I would rather there be a single
> react.js in the package and the version is whatever the build configuration
> is. Of course, if you actually need to load both variations, then we'll need
> both in the package. Do you?

No. I would like it to work that way too. Unfortunately, we can't do that with EXTRA_JS_MODULES. We would need the ability to "rename" a file. If you can show me how to add react.js at "devtools/shared/content/react.js" in a production build, but in a development instead put react-dev.js at that same place, let me know. I even asked in #build and they said you can't do that.

> 
> I'm also not keen on using MOZ_BRANDING_DIRECTORY as the thing we key off
> of. It works, but it isn't the proper abstraction layer. We really want a
> DEBUGGABLE_JS_MODULES flag or something. It's not that difficult to add one
> to configure.in. AC_SUBST makes a variable available to moz.build. Before
> you do that, it might be a good idea to see if we have an existing
> variable/flag we can repurpose.

I'm pretty far down a rabbit trail already. I don't have time to figure all that out in our build system, so either I need help doing it or we should just use MOZ_BRANDING_DIRECTORY for now.
(In reply to Mark Banner (:standard8) from comment #26)
> (In reply to Gregory Szorc [:gps] from comment #24)
> > If so, I'm not too keen on this solution. I would rather there be a single
> > react.js in the package and the version is whatever the build configuration
> > is. Of course, if you actually need to load both variations, then we'll need
> > both in the package. Do you?
> 
> Hello does this in jar.mn and keys off MOZ_DEBUG:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/jar.mn
> 
> Since the dev version of react is slower & does more checks, this seemed
> like a reasonable thing to do for us.

We thought of that but we really don't want to force devs to compile in debug mode, as it's so much slower. Frontend debug is way different than platform debug.
ni? gps again because I'd really like to know if you can figure out how to build in different dev/prod versions of a file into the exact same path
Flags: needinfo?(gps)
(In reply to James Long (:jlongster) from comment #29)
> ni? gps again because I'd really like to know if you can figure out how to
> build in different dev/prod versions of a file into the exact same path

See comment #26.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #30)
> (In reply to James Long (:jlongster) from comment #29)
> > ni? gps again because I'd really like to know if you can figure out how to
> > build in different dev/prod versions of a file into the exact same path
> 
> See comment #26.

We really, really don't want to use jar.mn. We are moving away from chrome URLs and mapping everything to a sane path on the filesystem. Using resource URLs consistently for everything is really nice, and easy to discover where things are loaded from (we are currently working on moving everything from devtools into a single directory).

What is your concern about exposing MOZ_BRANDING_DIRECTORY in AppConstants?
Oh, I see.

moz.build files don't yet expose a way to install files under a different filename via EXTRA_JS_MODULES. They could, if needed. But this seems like a lot of work for what appears to be a one-off.

How about having the moz.build add the appropriate file to EXTRA_JS_MODULES depending on MOZ_DEBUG or some such and then have consumers try to import one, catch the exception, then fall back to another? You could always write a shim js file that imports the appropriate file (or possibly #includes it via the preprocessor) and re-exports its symbols.

MOZ_BRANDING_DIRECTORY is a build configuration that says which *source* directory to grab branding files from. From the perspective of a built application, the branding files all exist in the same path (more or less). Using MOZ_BRANDING_DIRECTORY for this is a hack and a leaky abstraction.
(In reply to Gregory Szorc [:gps] from comment #32)
> Oh, I see.
> 
> moz.build files don't yet expose a way to install files under a different
> filename via EXTRA_JS_MODULES. They could, if needed. But this seems like a
> lot of work for what appears to be a one-off.

This may seem like a one-off, but we're really trying to reduce weirdness when it comes to loading JS files. To me, it seems really straight-forward to just include them in moz.build (potentially ignoring the dev version in prod if you want to save space). These one-offs tend to build up and create a lot of papercuts.

> How about having the moz.build add the appropriate file to EXTRA_JS_MODULES
> depending on MOZ_DEBUG or some such and then have consumers try to import
> one, catch the exception, then fall back to another? You could always write
> a shim js file that imports the appropriate file (or possibly #includes it
> via the preprocessor) and re-exports its symbols.

When would you do that exception/fall-back? Every single time you load React? Definitely don't want to do that. We could maybe do that in a custom loader but that seems like a ton of work, when all we just need is a runtime flag.

> 
> MOZ_BRANDING_DIRECTORY is a build configuration that says which *source*
> directory to grab branding files from. From the perspective of a built
> application, the branding files all exist in the same path (more or less).
> Using MOZ_BRANDING_DIRECTORY for this is a hack and a leaky abstraction.

Ok, I'm fine if we don't use that, but we also can't use MOZ_DEBUG. No way we want to run the full debug build just to get nice frontend errors.

I guess we really need to introduce a new JS variable like you mentioned before then?
Yeah, a new variable in configure.in is the way to go. It's pretty easy to add a variable. Search for MOZ_ARG_ENABLE_BOOL in configure.in. Use AC_SUBST to make the variable available to moz.build files.
Cool, thanks! What should the new variable be? You mentioned DEBUGGABLE_JS_MODULES. Other options: DEBUG_JS_MODULES, JS_DEBUG, FRONTEND_JS_DEBUG, FRONTEND_DEBUG, etc etc
I like DEBUG_JS_MODULES.
Attached patch 1181646.patchSplinter Review
This patch introduces the DEBUG_JS_MODULES config flag, that can be enabled with `ac_add_options --enable-debug-js-modules`. Eventually we should include that in the list of build options on whatever "getting started" page for devtools.

When that is enabled, the dev version of React will be bundled and the browser loader will automatically require it (without needing to change any `require`s)
Attachment #8631769 - Attachment is obsolete: true
Attachment #8640033 - Flags: review?(gps)
Attachment #8640033 - Flags: review?(gps) → review+
Depends on: 1190440
None of those failures should be related to this (this only adds some files and adds an option to the build process)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb43469cf706
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.