Closed Bug 1045690 Opened 6 years ago Closed 5 years ago

Get generated React formatting consistent to avoid whitespace changes

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
mozilla35
Iteration:
35.1
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: standard8, Unassigned)

Details

(Whiteboard: p=0.5 [no-loop-uplift])

Attachments

(1 file, 1 obsolete file)

This is probably an upstream bug.

When we're landing patches, some of them have different amounts of extra unnecessary whitespace and this is causing more complex diffs for the .js files.

We should probably file a bug on React to see if they can get it consistent. Assigning to Nicolas as I think he was looking at their bug database already.
So it appears that different versions of react-tools (the npm package providing the jsx executable which compiles our .jsx files to their regular .js counterpart) generate different coding styles:

> $ node_modules/.bin/jsx --version
> 0.10.0
> $ node_modules/.bin/jsx t.jsx
> built Module("t.jsx")
> /** @jsx React.DOM */
> var a = Foo( {p:"bar"});
> $ jsx --version
> 0.11.1
> $ jsx t.jsx
> built Module("t.jsx")
> /** @jsx React.DOM */
> var a = Foo({p: "bar"});

I think we should ensure we all use the very same version of the jsx executable, matching the one of the React lib currently used on the project (0.10.0 atm, 0.11.1 when bug 1040662 lands).

I've also filed a bug on React bt for the trailing whitespaces issue: https://github.com/facebook/react/issues/1954
Whiteboard: p=.5
Whiteboard: p=.5 → p=0.5
The suggestion at the meeting was just to stand up a test in build-jsx to ensure the person running it has the correct version installed.
Status: NEW → ASSIGNED
This adds a simple check that the version is what we expect. Although folks could use react by hand, this gives us some level of protection against using the wrong versions.
Attachment #8484938 - Flags: review?(dmose)
Assignee: nperriault → standard8
Iteration: --- → 35.1
Target Milestone: --- → mozilla35
Comment on attachment 8484938 [details] [diff] [review]
Ensure the correct version of react is used when building the Loop jsx files.

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

::: browser/components/loop/build-jsx
@@ +5,5 @@
>  import subprocess
>  from threading import Thread
>  import argparse
>  
> +REACT_VERSION="0.11.1"

Just wondering out loud, couldn't we parse the current react lib filename and extract the version number used, so we'd avoid having to maintain this concurrently?
Comment on attachment 8484938 [details] [diff] [review]
Ensure the correct version of react is used when building the Loop jsx files.

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

Redirecting review to Andrei...
Attachment #8484938 - Flags: review?(dmose) → review?(andrei.br92)
Whiteboard: p=0.5 → p=0.5[loop-uplift]
Are there any more changes you will bring to this patch regarding the way the version number is tested ?
Flags: needinfo?(standard8)
(In reply to Andrei Oprea [:andreio] from comment #6)
> Are there any more changes you will bring to this patch regarding the way
> the version number is tested ?

I can do that. I'll attach an updated patch.
Flags: needinfo?(standard8)
Updated patch so we extract REACT_VERSION out of the React javascript file itself.
Attachment #8484938 - Attachment is obsolete: true
Attachment #8484938 - Flags: review?(andrei.br92)
Attachment #8489902 - Flags: review?(standard8)
Comment on attachment 8489902 [details] [diff] [review]
Ensure the correct version of react is used when building the Loop jsx files.

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

Looks great. r=Standard8
Attachment #8489902 - Flags: review?(standard8) → review+
We don't need to uplift this, this is a NPOTB change.
Whiteboard: p=0.5[loop-uplift] → p=0.5
https://hg.mozilla.org/mozilla-central/rev/43a4d55b598e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify-
QA Contact: anthony.s.hughes
Whiteboard: p=0.5 → p=0.5 [no-loop-uplift]
Comment on attachment 8489902 [details] [diff] [review]
Ensure the correct version of react is used when building the Loop jsx files.

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8489902 - Flags: approval-mozilla-aurora?
Comment on attachment 8489902 [details] [diff] [review]
Ensure the correct version of react is used when building the Loop jsx files.

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 #8489902 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.