Remove flow types from the debugger
Categories
(DevTools :: Debugger, task, P3)
Tracking
(firefox88 fixed)
Tracking | Status | |
---|---|---|
firefox88 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 2 open bugs)
Details
Attachments
(3 files, 7 obsolete files)
Debugger panel is the only panel to use flow and this require:
- a build step, which:
- require to run
./mach build faster
before seeing the changes applied - doesn't support source map and makes it hard to understand stacktraces as locations aren't matching sources
- require to run
- a couple of non-trivial workarounds in order to address flow type system edge cases. Example
stringToSourceActorId
:
https://searchfox.org/mozilla-central/source/devtools/client/debugger/src/reducers/source-actors.js#176-182 - using yarn && yarn flow from debugger folder. Ideally, we should be able to work on any panel only by using ./mach commands.
Assignee | ||
Updated•4 years ago
|
Comment 1•4 years ago
|
||
I'm 100% in favor of removing flow.
(continuing the conversation on slack about losing a safety net when removing flow)
From what I remember, when we discussed debugger cleanups, we pushed back about removing the jest tests, because we were afraid we might lose coverage. I don't remember we had this argument about flow though.
Every time flow breaks, I update the types as quickly as I can to make it pass. It never "caught" any regression or issue for me.
I don't feel like flow brings much here. It's not well integrated in our tool chain and it's inconsistent with the rest of the code base.
Comment 2•4 years ago
|
||
I am also on board with removing it.
Honza
Updated•4 years ago
|
Comment 3•4 years ago
|
||
I feel the same, let's make our lives easier
Assignee | ||
Comment 4•4 years ago
|
||
Assignee | ||
Comment 5•4 years ago
|
||
This is an automated change, done via:
$ yarn global add flow-remove-type
$ ~/.yarn/bin/flow-remove-types devtools/client/debugger/src/ --out-dir devtools/client/debugger/src2
$ cp -r devtools/client/debugger/src2/* devtools/client/debugger/src/
Assignee | ||
Comment 6•4 years ago
|
||
This is also automated, via ./mach eslint --fix
Assignee | ||
Comment 7•4 years ago
|
||
./mach eslint
wasn't able to auto-address all issues.
I'm going over all of them here.
Assignee | ||
Comment 8•4 years ago
|
||
Remove references to flow in build files.
This effectively stop stipping flow from debugger modules.
So any leftover would now start being kept in production file and do an error.
Assignee | ||
Comment 9•4 years ago
|
||
Some manual fixes are necessary.
Especially lots of attributes being types in the beginning of classes,
which ended up overloading the real implementation of attributes/functions.
Assignee | ||
Comment 10•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
Assignee | ||
Comment 12•4 years ago
|
||
Assignee | ||
Comment 13•4 years ago
|
||
Assignee | ||
Comment 14•4 years ago
|
||
Assignee | ||
Comment 15•4 years ago
|
||
Updated•4 years ago
|
Comment 16•4 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/552023cfd750 [devtools] - Strip flow from the debugger sources. r=nchevobbe,bomsy https://hg.mozilla.org/integration/autoland/rev/3a94715fdff0 [devtools] - Remove eslint settings about flow. r=nchevobbe,bomsy https://hg.mozilla.org/integration/autoland/rev/828c62bfc0a2 [devtools] - Remove flow plugin and stop running flow on try r=nchevobbe,bomsy
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 17•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/552023cfd750
https://hg.mozilla.org/mozilla-central/rev/3a94715fdff0
https://hg.mozilla.org/mozilla-central/rev/828c62bfc0a2
Description
•