npm translator: Fix endless loop in case of dependency cycles #15
Conversation
|
|
codeworrior
force-pushed
the
avoid-endless-loop-for-dependency-cycles
branch
from
de9c8ac
to
3bb69f0
Compare
There was a problem hiding this comment.
I was wondering why I didn't implement something similar when we first faced the circular dependency issue last year. But I guess this solution only got possible due to a more recent refactoring that lead to the processPendingDeps function being implemented.
Could you please add a [FIX] prefix to the commit message and maybe remove the spaces in the if-statement to follow the general code style?
codeworrior
force-pushed
the
avoid-endless-loop-for-dependency-cycles
branch
from
3bb69f0
to
0d8ed08
Compare
|
|
||
| // Breadth-first search to prefer projects closer to root | ||
| while (queue.length) { | ||
| const project = queue.shift(); // Get and remove first entry from queue | ||
| if (!project.id) { | ||
| throw new Error("Encountered project with missing id"); | ||
| } | ||
| if (visited.has(project.id)) { |
There was a problem hiding this comment.
Would it make sense to to check for the combination of project.id and project.version instead of only the project.id?
There was a problem hiding this comment.
The projectPreprocessor does the same also on id-level: projectPreprocessor.js#L55. So I guess it wouldn't change much down that line.
While it is yet to be decided and documented, we once agreed (for the time being) to always prefer the projects closer to the root, ignoring the version.
There was a problem hiding this comment.
As per our Guidelines we prefer the commit message summary prefix to be uppercase 😉
Btw. I'm not sure whether our changelog would even include commits with a lower case prefix: .chglog/config.yml#L11
codeworrior
force-pushed
the
avoid-endless-loop-for-dependency-cycles
branch
from
0d8ed08
to
c5905b1
Compare
|
How could I miss that... |
Fixes https://github.com/SAP/ui5-project/issues/14 .