Closed Bug 1330779 Opened 7 years ago Closed 7 years ago

Use a bundle for reps in devtools

Categories

(DevTools :: Shared Components, enhancement, P2)

52 Branch
enhancement

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(4 files, 2 obsolete files)

Follow up to 1325401, reps have been moved to an experimental repo on Github : https://github.com/devtools-html/devtools-reps

This meta bug is to define the steps to officially move the reps from mc to github.
Looking at Bug 1325401, using reps from a bundle implies that we have to change the way we import reps:

> if (!Services.prefs.getBoolPref("devtools.webconsole.use-reps-bundle")) {
>     Rep = createFactories(require("devtools/client/shared/components/reps/rep")).Rep;
>     StringRep = createFactories(require("devtools/client/shared/components/reps/string").StringRep).rep;
>     Grip = require("devtools/client/shared/components/reps/grip").Grip;
>     MODE = require("devtools/client/shared/components/reps/constants").MODE;
> } else {
>     const bundle = require("devtools/client/shared/components/reps");
>     Rep = createFactory(bundle.Rep);
>     StringRep = createFactory(bundle.StringRep);
>     Grip = createFactory(bundle.Grip);
>     MODE = bundle.MODE;
> }

Here is a proposal:
- 1. Stop modifying reps on Github
- 2. Update reps in mc so that their export interface is the same as if they were in a bundle.
     We modify all call sites & tests to import reps, accordingly.
- 3.a. Copy reps from mc to github, cleanup the "define" wrappers etc...
- 3.b. Create a reps bundle
- 3.c. Delete reps from mc and use the bundle instead

The reason I propose this flow is to avoid relying on the preference from Bug 1325401 and having duplicated code to import reps in many files.

Nicolas, Jason: Let me know if I'm missing something, if you disagree etc...
Flags: needinfo?(jlaster)
Flags: needinfo?(chevobbe.nicolas)
Current call sites for reps in mc:
- client/dom
- client/jsonview
- client/netmonitor
- client/webconsole
- client/shared/components/test/

(and an import to reps.css in client/shared/components/tree/tree-view.css ...)
After dicussing last week, here is an update on the path to move reps to github:

#1 - [MC] Land Bug 1325401, which contains an experimental reps bundle that can be used in mc.
     After this bug, it becomes transparent for all call sites whether we use a bundle or the mc reps.
#2 - [GH] Fix Launchpad issue https://github.com/devtools-html/devtools-core/issues/91, that prevents building the reps bundle
#3 - [GH] Run mochitests in the github repo (https://github.com/devtools-html/devtools-reps/issues/1 ?)
#4 - [GH] Decide an updating calendar & process to regularly update the reps bundle in mc
#5 - [GH] Synchronize Github reps with the changes coming from mc
#6 - [MC] Copy reps bundle & assets to MC, delete old reps, tests and resources

One thing to note is that we don't have any plan to preserve the mc commit history here. Will raise this point during next Github status.

Clearing ni?s since this was discussed already.
Flags: needinfo?(jlaster)
Flags: needinfo?(chevobbe.nicolas)
Depends on: 1325401
Depends on: 1332068
With https://github.com/devtools-html/devtools-reps/pull/57 merged on devtools-reps, we are now running reps mochitests using the bundle on every pull request.

We should now test using the reps bundle for a full try run.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7b2af6ae0307d78306ce996778946b3698ef88

Some background that led to this:

Started simply by updating the tests, the bundle, and flipping on the bundle in load-reps.js.
Beyond some eslint issues, an initial try push seems green.
-> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5032c5504567d04f02240c2a754475c862278218

After that I tried removing the existing reps files from mc. This made the json viewer fail
-> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cce62ef20c9a9e756343cad1c0a6ac5f47cc1dcd

Turns out requirejs is aggressively fetching all the files required in load-reps.js even if the require is behind an if (false) statement. So for now I commented out the requires. This leads me to think that we should probably simplify the load-reps.js to make it a simple wrapper as soon as we remove the existing reps from mc. We can't really "flip it back" anyway if the files are deleted.

And I really would like to avoid having stale sources in mc if we decide to go for Github officially.
Comment on attachment 8831647 [details]
Bug 1330779 - Update reps tests to use load-reps;

https://reviewboard.mozilla.org/r/108200/#review109670

r+ if try is green :)
Attachment #8831647 - Flags: review?(chevobbe.nicolas) → review+
Ah nice catch, I did fixed it on Github but not here. Updated the changeset
Depends on: 1335050
Assignee: nobody → jdescottes
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P2
Summary: [meta] Use a bundle for reps in devtools → Use a bundle for reps in devtools
Comment on attachment 8831648 [details]
Bug 1330779 - reps 0.1.0: update reps bundle, files and tests from github;

https://reviewboard.mozilla.org/r/108202/#review112748
Attachment #8831648 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8831649 [details]
Bug 1330779 - reps 0.1.0: remove reps files from mc;

https://reviewboard.mozilla.org/r/108204/#review112752

\o/
Attachment #8831649 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8831650 [details]
Bug 1330779 - comment out requires in load-reps;

https://reviewboard.mozilla.org/r/108206/#review112754
Attachment #8831650 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8835590 [details]
Bug 1330779 - reps 0.1.0: update reps tests from github;

https://reviewboard.mozilla.org/r/111280/#review112756
Attachment #8835590 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8835624 [details]
Bug 1330779 - reps 0.1.0: add a README file in devtools reps folder;

https://reviewboard.mozilla.org/r/111294/#review112758
Attachment #8835624 - Flags: review?(chevobbe.nicolas) → review+
Attachment #8835590 - Attachment is obsolete: true
Attachment #8831650 - Attachment is obsolete: true
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ad03e900252
Update reps tests to use load-reps;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/925f3f0167c3
reps 0.1.0: update reps bundle, files and tests from github;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/c71db570f995
reps 0.1.0: remove reps files from mc;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/73b628209894
reps 0.1.0: add a README file in devtools reps folder;r=nchevobbe
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: