Skip to content

Commit

Permalink
bug #841 Refactor PackageJsonSynchronizer to prevent unintentional du…
Browse files Browse the repository at this point in the history 
…plicate dependencies (codedmonkey)

This PR was squashed before being merged into the 1.x branch.

Discussion
----------

Refactor PackageJsonSynchronizer to prevent unintentional duplicate dependencies

Fixes #840

Changes how `PackageJsonSynchronizer` handles dependency resolving. If a dependency is already defined under the `dependencies` section of package.json, Flex won't add it again under `devDependencies`. If multiple UX bundles require the same dependency, no action is performed.

Because the way `PackageJsonSynchronizer` was 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.

Commits
-------

815c96f Refactor PackageJsonSynchronizer to prevent unintentional duplicate dependencies
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Jan 27, 2022
2 parents d1ca6ac + 815c96f commit 441f671
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 89 deletions.
163 changes: 76 additions & 87 deletions src/PackageJsonSynchronizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@

use Composer\Json\JsonFile;
use Composer\Json\JsonManipulator;
use Composer\Semver\Constraint\ConstraintInterface;
use Composer\Semver\Intervals;
use Composer\Semver\VersionParser;
use Seld\JsonLint\ParsingException;

/**
Expand All @@ -40,14 +37,17 @@ public function shouldSynchronize(): bool

public function synchronize(array $phpPackages): bool
{
// Remove all links and add again only the existing packages
try {
$didAddLink = $this->removePackageJsonLinks((new JsonFile($this->rootDir.'/package.json'))->read());
JsonFile::parseJson(file_get_contents($this->rootDir.'/package.json'));
} catch (ParsingException $e) {
// if package.json is invalid (possible during a recipe upgrade), we can't update the file
return false;
}

$didChangePackageJson = $this->removeObsoletePackageJsonLinks();

$dependencies = [];

foreach ($phpPackages as $k => $phpPackage) {
if (\is_string($phpPackage)) {
// support for smooth upgrades from older flex versions
Expand All @@ -56,22 +56,29 @@ public function synchronize(array $phpPackages): bool
'keywords' => ['symfony-ux'],
];
}
$didAddLink = $this->addPackageJsonLink($phpPackage) || $didAddLink;

foreach ($this->resolvePackageDependencies($phpPackage) as $dependency => $constraint) {
$dependencies[$dependency][$phpPackage['name']] = $constraint;
}
}

$this->registerPeerDependencies($phpPackages);
$didChangePackageJson = $this->registerDependencies($dependencies) || $didChangePackageJson;

// Register controllers and entrypoints in controllers.json
$this->registerWebpackResources($phpPackages);

return $didAddLink;
return $didChangePackageJson;
}

private function removePackageJsonLinks(array $packageJson): bool
private function removeObsoletePackageJsonLinks(): bool
{
$didRemoveLink = false;
$jsDependencies = $packageJson['dependencies'] ?? [];
$jsDevDependencies = $packageJson['devDependencies'] ?? [];
$didChangePackageJson = false;

$manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
$content = json_decode($manipulator->getContents(), true);

$jsDependencies = $content['dependencies'] ?? [];
$jsDevDependencies = $content['devDependencies'] ?? [];

foreach (['dependencies' => $jsDependencies, 'devDependencies' => $jsDevDependencies] as $key => $packages) {
foreach ($packages as $name => $version) {
Expand All @@ -82,39 +89,78 @@ private function removePackageJsonLinks(array $packageJson): bool
continue;
}

$manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
$manipulator->removeSubNode('devDependencies', $name);
file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());
$didRemoveLink = true;
$manipulator->removeSubNode($key, $name);
$didChangePackageJson = true;
}
}

return $didRemoveLink;
file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());

return $didChangePackageJson;
}

private function addPackageJsonLink(array $phpPackage): bool
private function resolvePackageDependencies($phpPackage): array
{
$dependencies = [];

if (!$packageJson = $this->resolvePackageJson($phpPackage)) {
return false;
return $dependencies;
}

$manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
$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);

foreach ($packageJson->read()['peerDependencies'] ?? [] as $peerDependency => $constraint) {
$dependencies[$peerDependency] = $constraint;
}

return $dependencies;
}

private function registerDependencies(array $flexDependencies): bool
{
$didChangePackageJson = false;

$manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
$content = json_decode($manipulator->getContents(), true);

$devDependencies = $content['devDependencies'];
uksort($devDependencies, 'strnatcmp');
$manipulator->addMainKey('devDependencies', $devDependencies);
foreach ($flexDependencies as $dependency => $constraints) {
if (1 !== \count($constraints) && 1 !== \count(array_count_values($constraints))) {
// If the flex packages have a colliding peer dependency, leave the resolution to the user
continue;
}

$constraint = array_shift($constraints);

$newContents = $manipulator->getContents();
if ($newContents === file_get_contents($this->rootDir.'/package.json')) {
return false;
$parentNode = isset($content['dependencies'][$dependency]) ? 'dependencies' : 'devDependencies';
if (!isset($content[$parentNode][$dependency])) {
$content['devDependencies'][$dependency] = $constraint;
$didChangePackageJson = true;
} elseif ($constraint !== $content[$parentNode][$dependency]) {
$content[$parentNode][$dependency] = $constraint;
$didChangePackageJson = true;
}
}

file_put_contents($this->rootDir.'/package.json', $newContents);
if ($didChangePackageJson) {
if (isset($content['dependencies'])) {
$manipulator->addMainKey('dependencies', $content['dependencies']);
}

if (isset($content['devDependencies'])) {
$devDependencies = $content['devDependencies'];
uksort($devDependencies, 'strnatcmp');
$manipulator->addMainKey('devDependencies', $devDependencies);
}

$newContents = $manipulator->getContents();
if ($newContents === file_get_contents($this->rootDir.'/package.json')) {
return false;
}

file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());
}

return true;
return $didChangePackageJson;
}

private function registerWebpackResources(array $phpPackages)
Expand Down Expand Up @@ -151,7 +197,7 @@ private function registerWebpackResources(array $phpPackages)
continue;
}

// Otherwise, the package exists: merge new config with uer config
// Otherwise, the package exists: merge new config with user config
$previousConfig = $previousControllersJson['controllers'][$name][$controllerName];

$config = [];
Expand Down Expand Up @@ -180,39 +226,6 @@ private function registerWebpackResources(array $phpPackages)
file_put_contents($controllersJsonPath, json_encode($newControllersJson, \JSON_PRETTY_PRINT | \JSON_UNESCAPED_SLASHES)."\n");
}

public function registerPeerDependencies(array $phpPackages)
{
$peerDependencies = [];

foreach ($phpPackages as $phpPackage) {
if (!$packageJson = $this->resolvePackageJson($phpPackage)) {
continue;
}

$versionParser = new VersionParser();

foreach ($packageJson->read()['peerDependencies'] ?? [] as $peerDependency => $constraint) {
$peerDependencies[$peerDependency][$constraint] = $versionParser->parseConstraints($constraint);
}
}

if (!$peerDependencies) {
return;
}

$manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
$content = json_decode($manipulator->getContents(), true);
$devDependencies = $content['devDependencies'] ?? [];

foreach ($peerDependencies as $peerDependency => $constraints) {
$devDependencies[$peerDependency] = $this->compactConstraints($constraints);
}
uksort($devDependencies, 'strnatcmp');
$manipulator->addMainKey('devDependencies', $devDependencies);

file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());
}

private function resolvePackageJson(array $phpPackage): ?JsonFile
{
$packageDir = $this->rootDir.'/'.$this->vendorDir.'/'.$phpPackage['name'];
Expand All @@ -233,28 +246,4 @@ private function resolvePackageJson(array $phpPackage): ?JsonFile

return null;
}

/**
* @param ConstraintInterface[] $constraints
*/
private function compactConstraints(array $constraints): string
{
if (method_exists(Intervals::class, 'isSubsetOf')) {
foreach ($constraints as $k1 => $constraint1) {
foreach ($constraints as $k2 => $constraint2) {
if ($k1 !== $k2 && Intervals::isSubsetOf($constraint1, $constraint2)) {
unset($constraints[$k2]);
}
}
}
}

uksort($constraints, 'strnatcmp');

foreach ($constraints as $k => $constraint) {
$constraints[$k] = \count($constraints) > 1 && false !== strpos($k, '|') ? '('.$k.')' : $k;
}

return implode(',', $constraints);
}
}
15 changes: 15 additions & 0 deletions tests/Fixtures/packageJson/elevated_dependencies_package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"name": "symfony/fixture",
"dependencies": {
"@hotcookies": "^1.1|^2",
"@hotdogs": "^2",
"@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets"
},
"devDependencies": {
"@symfony/stimulus-bridge": "^1.0.0",
"stimulus": "^1.1.1"
},
"browserslist": [
"defaults"
]
}
34 changes: 32 additions & 2 deletions tests/PackageJsonSynchronizerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace Symfony\Flex\Tests;

use Composer\Semver\Intervals;
use PHPUnit\Framework\TestCase;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Flex\PackageJsonSynchronizer;
Expand Down Expand Up @@ -145,7 +144,6 @@ public function testSynchronizeNewPackage()
'{
"name": "symfony/fixture",
"devDependencies": {
"@hotcookies": "'.(method_exists(Intervals::class, 'isSubsetOf') ? '^1.1' : '^1.1,(^1.1|^2)').'",
"@hotdogs": "^2",
"@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets",
"@symfony/new-package": "file:vendor/symfony/new-package/assets",
Expand Down Expand Up @@ -215,4 +213,36 @@ public function testArrayFormattingHasNotChanged()
trim(file_get_contents($this->tempDir.'/package.json'))
);
}

public function testExistingElevatedPackage()
{
(new Filesystem())->copy($this->tempDir.'/elevated_dependencies_package.json', $this->tempDir.'/package.json', true);

$this->synchronizer->synchronize([
[
'name' => 'symfony/existing-package',
'keywords' => ['symfony-ux'],
],
]);

// Should keep existing package references and config
$this->assertSame(
[
'name' => 'symfony/fixture',
'dependencies' => [
'@hotcookies' => '^1.1|^2',
'@hotdogs' => '^2',
'@symfony/existing-package' => 'file:vendor/symfony/existing-package/Resources/assets',
],
'devDependencies' => [
'@symfony/stimulus-bridge' => '^1.0.0',
'stimulus' => '^1.1.1',
],
'browserslist' => [
'defaults',
],
],
json_decode(file_get_contents($this->tempDir.'/package.json'), true)
);
}
}

0 comments on commit 441f671

Please sign in to comment.