Refactor PackageJsonSynchronizer to prevent unintentional duplicate dependencies #841
Conversation
codedmonkey
changed the title
src/PackageJsonSynchronizer.php
Outdated
| $manipulator->addSubNode('devDependencies', '@'.$phpPackage['name'], 'file:'.substr($packageJson->getPath(), 1 + \strlen($this->rootDir), -13)); | ||
| $dependencies['@'.$phpPackage['name']] = 'file:'.substr($packageJson->getPath(), 1 + \strlen($this->rootDir), -13); | ||
|
|
||
| $versionParser = new VersionParser(); |
There was a problem hiding this comment.
Relying on the Composer version parser to parse npm constraints looks wrong to me. The syntax is not the same between both ecosystems (some parts of the syntax are common, but not everything)
There was a problem hiding this comment.
and worse than that. Some syntax is the same with different meaning (so the parser would accept it but produce a different result)
There was a problem hiding this comment.
I agree it's a problem. In the current version it simply generates an invalid package.json the developer has to fix themselves. This hasn't been a common use case for Flex yet so it rarely occurs. What is the intended behavior?
There was a problem hiding this comment.
Can you explain the problem a bit more that this is solving? Is this if you install 2 packages that both have chart.js has a peer dependency... but they one requires v2 and another requires v3?
If so, yea... it's super rare. Well, probably not a reality at all right now. I would vote to simply "do nothing". What I mean is, if you already had chart.js in your package.json, leave it with whatever version was there. This is a problem the user will need to resolve and they will see peer dependency warnings from yarn/npm.
There was a problem hiding this comment.
I wasn't trying to solve it but while writing the PR I noticed it simply generated invalid JSON with 2 conflicting peer dependencies.
Apperantly I forgot the use case for peer dependencies in NPM so awesome suggestion. Thanks :)
|
Apologies for the delay, I had a very persistent cold the last few weeks. |
codedmonkey
changed the title
codedmonkey
changed the title
|
Can you please rebase? |
|
/cc @tgalopin could you please have a look? |
codedmonkey
force-pushed
the
json-elevated
branch
3 times, most recently
from
023a72e
to
db23b8d
Compare
nicolas-grekas
force-pushed
the
json-elevated
branch
from
db23b8d
to
815c96f
Compare
|
Thank you @codedmonkey. |
Fixes #840
Changes how
PackageJsonSynchronizerhandles dependency resolving. If a dependency is already defined under thedependenciessection of package.json, Flex won't add it again underdevDependencies. If multiple UX bundles require the same dependency, no action is performed.Because the way
PackageJsonSynchronizerwas written, I had to change the order of the steps in the synchronization process. While it used to update the package.json for each UX dependency individually, it now resolves a list of dependencies and updates package.json at the end.I also changed how it handles incompatible peer dependencies, no action is performed by the synchronizer if multiple ux package require incompatible peer dependencies.