Refactor export map builder #2991
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2991 +/- ##
==========================================
+ Coverage 81.81% 82.45% +0.63%
==========================================
Files 77 86 +9
Lines 3795 3813 +18
Branches 1257 1256 -1
==========================================
+ Hits 3105 3144 +39
+ Misses 690 669 -21 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks, this is great!
It seems like this refactor is exposing a lot of previously uncovered lines. It's fine to land this without covering them, but it would be even better if this PR (or another one, landed first) could include additional tests so we can ensure the uncovered lines are all covered
src/exportMap/captureDependency.js
Outdated
| @@ -0,0 +1,60 @@ | |||
| export function captureDependency( | |||
There was a problem hiding this comment.
| export function captureDependency( | |
| export default function captureDependency( |
There was a problem hiding this comment.
Why would captureDependency be the default and not, say, captureDependencyWithSpecifiers that is exported from the same file?
There was a problem hiding this comment.
because the filename is "captureDependency". a default export is what a module is, a named export is something a module has, and most modules should be something.
I reviewed the lines that aren't covered. |
|
Has been merged via |
ljharb
force-pushed
the
refactor-export-map-builder
branch
from
6d8a0b8
to
8587c85
Compare
exportMapBuilder.js contains a huge god-object with 651 lines of code, with tightly coupled procedures that are hard to follow.
Lets extract procedures and classes, and put them in well named files.
This separation will also help with encapsulation and decoupling.
It's not perfect yet, but a reduction from 651 lines to 207 lines is a 68% decrease.