Closed
Bug 1330779
Opened 7 years ago
Closed 7 years ago
Use a bundle for reps in devtools
Categories
(DevTools :: Shared Components, enhancement, P2)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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 ...)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
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 10•7 years ago
|
||
mozreview-review |
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+
Comment 11•7 years ago
|
||
there's some eslint error https://treeherder.mozilla.org/#/jobs?repo=try&revision=44e65b9acf4ea3e67b1a38f2d058309a6f38300a&selectedJob=73345070
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
Ah nice catch, I did fixed it on Github but not here. Updated the changeset
Assignee | ||
Updated•7 years ago
|
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
mozreview-review |
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 27•7 years ago
|
||
mozreview-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 28•7 years ago
|
||
mozreview-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 29•7 years ago
|
||
mozreview-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 30•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8835590 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8831650 -
Attachment is obsolete: true
Comment 34•7 years ago
|
||
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
Comment 35•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ad03e900252 https://hg.mozilla.org/mozilla-central/rev/925f3f0167c3 https://hg.mozilla.org/mozilla-central/rev/c71db570f995 https://hg.mozilla.org/mozilla-central/rev/73b628209894
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Assignee | ||
Updated•7 years ago
|
Blocks: reps-bundle-updates
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•