Closed Bug 1035655 Opened 5 years ago Closed 5 years ago

Define a strategy to handle development & production environments for React on desktop embedding

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: NiKo, Assigned: dmose)

References

Details

(Whiteboard: [Loop-uplift])

Attachments

(1 file, 1 obsolete file)

Right now we're using the development version of React in Loop. What we'd really want is:

- use the production version for the user facing application (desktop & standalone)
- use the development version for tests
- being able to switch to using the development version locally, eg. using a flag or setting, for debugging
Assignee: nobody → dmose
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8480255 [details] [diff] [review]
Use the production build of React for non-DEBUG builds

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

This patch adds and uses the production build for the desktop builds which are not DEBUG.  It's notably snappier, even in a desktop build.  

To fix the standalone side, we'll need to wait until we have a standalone build process (which we're getting in bug 1044411 in the form of grunt).
Attachment #8480255 - Flags: review?(standard8)
Comment on attachment 8480255 [details] [diff] [review]
Use the production build of React for non-DEBUG builds

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

This doesn't work for debug builds - you get the production react. I think you need to add "#filter substitution" at the start of the jar.mn file.

::: browser/components/loop/jar.mn
@@ +49,5 @@
>    content/browser/loop/shared/js/utils.js             (content/shared/js/utils.js)
>    content/browser/loop/shared/js/websocket.js         (content/shared/js/websocket.js)
>  
>    # Shared libs
> +  #ifdef DEBUG

Please put the ifdef stuff at the start of the line with no indent, as it makes it clear for find out what is/isn't included.
Attachment #8480255 - Flags: review?(standard8) → review-
Attachment #8480255 - Attachment is obsolete: true
Attachment #8483694 - Flags: review?(standard8)
(In reply to Mark Banner (:standard8) from comment #3)
> Comment on attachment 8480255 [details] [diff] [review]
> Use the production build of React for non-DEBUG builds
> 
> Review of attachment 8480255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't work for debug builds - you get the production react.

For me at least, it does work for debug builds and production.  I can switch "#ifdef DEBUG" to "#ifndef DEBUG" and the version will change, as verified by the browser toolbox debugger.

Once, at least, I made the mistake of pasting the link in the same instance of the browser and inspecting that with the debugger.  This inadvertently tripped me up, because the debugger happened to see the standalone content version of react first (rather than the embedded version of the react), and this patch only addresses the browser-embedded version.

> I think
> you need to add "#filter substitution" at the start of the jar.mn file.

If you look at mxr, there are other uses of #ifdef in the tree, and they don't need this.
 
> ::: browser/components/loop/jar.mn
> @@ +49,5 @@
> >    content/browser/loop/shared/js/utils.js             (content/shared/js/utils.js)
> >    content/browser/loop/shared/js/websocket.js         (content/shared/js/websocket.js)
> >  
> >    # Shared libs
> > +  #ifdef DEBUG
> 
> Please put the ifdef stuff at the start of the line with no indent, as it
> makes it clear for find out what is/isn't included.

Fixed; new patch attached.
Comment on attachment 8483694 [details] [diff] [review]
Use the production build of React for non-DEBUG builds

Ok, this seems to work now that I've retried it.
Attachment #8483694 - Flags: review?(standard8) → review+
Blocks: 1062959
Summary: Define a strategy to handle development & production environments for React → Define a strategy to handle development & production environments for React on desktop embedding
Whiteboard: [Loop-uplift]
https://hg.mozilla.org/mozilla-central/rev/9ef1c5956a05
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Untracking this for QE verification. Please needinfo me to request testing.
Flags: qe-verify-
QA Contact: anthony.s.hughes
Comment on attachment 8483694 [details] [diff] [review]
Use the production build of React for non-DEBUG builds

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8483694 - Flags: approval-mozilla-aurora?
Comment on attachment 8483694 [details] [diff] [review]
Use the production build of React for non-DEBUG builds

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.
Attachment #8483694 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.