feat(optional): ignore failed optional deps #27
Conversation
zkat
force-pushed
the
zkat/optional-fail
branch
3 times, most recently
from
4bf6684
to
da76503
Compare
zkat
changed the title
zkat
force-pushed
the
zkat/optional-fail
branch
2 times, most recently
from
19e3a08
to
f93b3d6
Compare
index.js
Outdated
| for (let dep of tree.dependencies.values()) { | ||
| if (dep.seenByMark) { continue } | ||
| dep.seenByMark = true | ||
| if (!dep.optional || !dep.failed) { |
There was a problem hiding this comment.
At this point, shouldn’t the only failed deps be optional, otherwise they would have thrown in the extractTree phase before this?
There was a problem hiding this comment.
I can probably get rid of that dep.optional. I think it's an artifact of an experiment, and pretty much useless now.
There was a problem hiding this comment.
This seems like the only piece of feedback to incorporate.
| @@ -137,11 +138,58 @@ class Installer { | |||
| this.pkgCount++ | |||
| return this | |||
| }) | |||
| .catch(e => { | |||
| if (child.optional) { | |||
| this.pkgCount++ | |||
There was a problem hiding this comment.
Should we be incrementing pkgCount for optional deps?
There was a problem hiding this comment.
Nvm, I see it’s decremented later.
index.js
Outdated
| .catch(e => { | ||
| if (child.optional) { | ||
| this.pkgCount++ | ||
| child.failed = true |
There was a problem hiding this comment.
Rather than marking this as failed via a property, could you do a Set for failed here? I know that in the mark phase, you’re iterating over the top level dependencies only to find failed deps, so a Set doesn’t save much, but it’s something.
index.js
Outdated
| if (dep.seenByMark) { continue } | ||
| dep.seenByMark = true | ||
| if (!dep.optional || !dep.failed) { | ||
| liveDeps.add(dep) |
There was a problem hiding this comment.
You may be able to eliminate this mark phase if you add to a failed Set instead of doing dep.failed during the extract phase...
index.js
Outdated
| if (dep.seenBySweep) { return } | ||
| dep.seenBySweep = true | ||
| return sweep(dep).then(() => { | ||
| if (!liveDeps.has(dep) && !dep.purged) { |
There was a problem hiding this comment.
If there was a failed Set as suggested above, you could invert this conditional as if (failedDeps.has(dep))
There was a problem hiding this comment.
how would you mark transitive deps that didn't themselves fail?
index.js
Outdated
| dep.address.replace(/:/g, '/node_modules/') | ||
| ) | ||
| installer.pkgCount-- | ||
| dep.purged = true |
There was a problem hiding this comment.
Rather than marking as purged, if there was a failed Set, you could remove it from the Set here, and not have to track purged at all, I think.
There was a problem hiding this comment.
We're doing a tree crawl through the whole thing anyway. This wouldn't really affect much.
|
I responded in more detail on Discord but the tl;dr is that using a GC method makes it super easy to make sure we're purging unreachable transitive deps of failed optional deps, and taking into account that they might not be optional for others. The performance impact of this GC is negligible at best: on the order of 10-20ms on a project with 2k+ deps |
zkat
force-pushed
the
zkat/optional-fail
branch
from
f93b3d6
to
c1c06e8
Compare
zkat
force-pushed
the
zkat/optional-fail
branch
from
c1c06e8
to
bc91f16
Compare
Fixes: #3