[AssetMapper] Warn of missing or incompat dependencies #51849

weaverryan · 2023-10-04T18:48:21Z

Q	A
Branch?	6.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	None
License	MIT

Hi!

The aim of the importmap.php system is to be a simple way to manage your JS dependencies. But to make it robust enough for production use, it does need a few things - like the importmap:audit command in #51650. This PR adds a check, during importmap:require and importmap:update, that reports any missing dependencies or dependencies with invalid versions. This is necessary so that, if package A requires package B, their versions don't "drift" over time without you being aware (e.g. you update package A to v3 but keep package B at v1, even though v3 of A requires v2 of B).

Built on top of #51786.

Cheers!

ahmedyakoubi · 2023-10-04T19:10:36Z

why not installing the missing dependency automatically instead of doing it manually?

weaverryan · 2023-10-04T19:26:21Z

why not installing the missing dependency automatically instead of doing it manually?

They will be installed automatically when you first require the package - e.g. importmap:require bootstrap will also grab @popper/core. So, if you ever see the but it is not in the importmap.php message from this PR, something has gone wrong or you've removed something by hand :)

weaverryan · 2023-10-06T18:07:11Z

src/Symfony/Component/AssetMapper/ImportMap/Resolver/PackageResolverInterface.php

@@ -35,7 +35,7 @@ public function resolvePackages(array $packagesToRequire): array;
     *
     * @param array<string, ImportMapEntry> $importMapEntries
     *
-     * @return array<string, string>
+     * @return array<string, array{content: string, dependencies: string[]}>


Not ideal... not sure if it merits a new object - as this is entirely internal, unless someone makes their own package resolver.

src/Symfony/Component/AssetMapper/composer.json

weaverryan · 2023-10-06T18:08:23Z

src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapVersionChecker.php

+            return substr($importName, 0, $i);
+        }
+
+        return $importName;


A bit of duplication with JsDelivrEsmResolver, but we have a plan to centralize this in a future PR.

src/Symfony/Bundle/FrameworkBundle/Resources/config/asset_mapper.php

src/Symfony/Component/AssetMapper/ImportMap/PackageVersionProblem.php

src/Symfony/Component/AssetMapper/ImportMap/ImportMapVersionChecker.php

fabpot · 2023-10-08T06:26:02Z

src/Symfony/Component/AssetMapper/ImportMap/ImportMapVersionChecker.php

+                $dependencyVersionConstraint = $packageDependencies[$dependencyName] ?? null;
+
+                if (null === $dependencyVersionConstraint) {
+                    // odd that this would happen


It makes me nervous 😂

Haha, good point - that comment was a bit ominous. It would be an unexpected but not dangerous situation where we see that a vendor file has import 'foo', but then when we check the metadata for that package, it doesn't list foo as a dependency at all. I have no idea if this will ever happen under normal circumstances - I can't think of a way (some misconfigured package ?).

But, to be safe, I've changed this to also issue a warning (like the other warnings) so the user is aware of it.

stof · 2023-10-16T10:52:11Z

src/Symfony/Component/AssetMapper/composer.json

@@ -17,6 +17,7 @@
    ],
    "require": {
        "php": ">=8.1",
+        "composer/semver": "^3.0",


Be careful about that. The constraints are not 100% compatible between the npm and composer ecosystems.

Hmm, is there an alternative? Checking into this quickly, differences I found:

npm has a 1.0.0 - 2.0.0 syntax.

npm seems to have a 5.4.x syntax which means 5.4.*.

I think this feature is pretty important, without it, it gets easy for your dependencies to "drift" out of sync (e.g. you install boostrap today and we also give you @popper/core, then you upgrade bootstrap but forget to upgrade popper and have no warning). So perhaps, IF we run into problems with odd, npm-specific dependency things we can handle those manually? Or maybe we can handle the above 2 items right now manually.

EDIT: I'll add a bit of pre-processing and tests to cover the edge-case differences.

IIRC, the ~ operator does not work the same either

I've added a simple conversion system using all of the differences I could find. There are some edge-case things that can't be converted - e.g. stable or https://example.com/file.js, but I think that's ok. This is just a system to warn users of potential problems (like the peer dependency warning in npm). And if we're missing some real-world cases, we can find out about them and patch them. The "conversion" code is fairly minimal, as the differences don't appear to be too many.

Thanks for catching this!

weaverryan · 2023-10-17T01:19:15Z

~~Failing tests coming from #50734~~

weaverryan · 2023-10-18T13:07:02Z

@stof I'd love your thoughts on this PR now. A few notes:

A) I do think this PR is important, so that your dependency versions don't drift away from each other. We're not building a full-blown package manager, but this seems like a pragmatic precaution.

B) I think i've covered the npm-specific version constraint differences nicely

C) In practice, it seems that when some packages are published, their dependencies in package.json change to exact versions. An example is vue - notice @vue/runtime-dom in https://registry.npmjs.org/vue/3.3.0 is listed at exactly 3.3.0. This isn't a problem, it just looks a bit odd and I wanted to mention it (and thought you might have even more insight).

But, I do still think this PR is ready to go

stof · 2023-10-18T13:22:54Z

For C, it is a choice of each package maintainer.
Some JS ED4F maintainers prefer to require exact versions of their dependencies. Unlike in composer, this does not create a dependency hell as the npm ecosystem does not require a flat dependency list in the installed dependencies with a single version of each package (Composer requires that for PHP package because PHP defines all classes in the same global scope so you cannot have 2 different version of the same package loaded. But JS works with a concept of modules exporting things, not with a shared naming scope).
There are mostly 2 cases there:

several related packages from the same maintainer having a shared release cycle (often in a mono-repo): exact version constraint avoids the need to deal with the case of having an updated version of one package while others are not updated. Especially useful when the public API of one package actually relies on the other package
dependencies on unrelated packages. For those, I don't have an argument in favor of it (as I'm actually in the other side there). The big drawback is that this requires actively monitoring releases of the dependencies to update them even when they release patch versions instead of relying on semver)

Note that the fact that the JS ecosystem does not require flat dependencies means that it might be impossible to resolve dependencies in a flat way (and doing that requires a composer-like solver anyway and would fail in cases of a dependency hell). And this means that your installer should be able to support the case of having several versions of the same package to respect constraints otherwise it might cause issues.

weaverryan · 2023-10-18T13:30:13Z

That's exactly the insight I was hoping for! 🙏

Note that the fact that the JS ecosystem does not require flat dependencies means that it might be impossible to resolve dependencies in a flat way. And this means that your installer should be able to support the case of having several versions of the same package to respect constraints otherwise it might cause issues

I think this is possible, via the importmap scopes. But I'd like to see if we actually need this. The consequence is that, right now, the system will work in the simple, "flat dependencies" kind of way. And I think that's fine. This PR would at least help users discover this potential problem more readily, then we can chat in the future about solving it.

nicolas-grekas · 2023-10-19T15:16:06Z

src/Symfony/Component/AssetMapper/composer.json

@@ -17,6 +17,7 @@
    ],
    "require": {
        "php": ">=8.1",
+        "composer/semver": "^3.0",


~~What about making this an optional dep and throw a useful LogicException in the command when used?~~

nvm, it's not an optional feat

I thought about that... but we don't use it in a specific command, we run this in the background when using importmap:require or importmap:update. So in practice, it WILL be needed. And if we just didn't show the warning if this wasn't installed, then nobody would have it installed and it wouldn't do anything. I couldn't find a nice way to avoid it :)

src/Symfony/Component/AssetMapper/ImportMap/ImportMapVersionChecker.php

nicolas-grekas · 2023-10-19T15:21:39Z

Thank you @weaverryan.

carsonbot added Status: Needs Review AssetMapper Feature labels Oct 4, 2023

carsonbot added this to the 6.4 milestone Oct 4, 2023

weaverryan added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 6, 2023

weaverryan force-pushed the asset-mapper-warn-incompat branch 3 times, most recently from 7fefbbe to 0ad63e7 Compare October 6, 2023 18:10

weaverryan commented Oct 6, 2023

View reviewed changes

fabpot requested changes Oct 7, 2023

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/asset_mapper.php Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 7, 2023

weaverryan force-pushed the asset-mapper-warn-incompat branch 2 times, most recently from 588a4df to c7df5dc Compare October 7, 2023 17:59

fabpot reviewed Oct 8, 2023

View reviewed changes

weaverryan force-pushed the asset-mapper-warn-incompat branch from c7df5dc to d95d406 Compare October 8, 2023 14:16

weaverryan force-pushed the asset-mapper-warn-incompat branch from 5e7a66d to db17147 Compare October 16, 2023 10:46

weaverryan added Status: Needs Review and removed Status: Needs Work labels Oct 16, 2023

stof reviewed Oct 16, 2023

View reviewed changes

weaverryan force-pushed the asset-mapper-warn-incompat branch 2 times, most recently from d88507f to 00f269f Compare October 17, 2023 01:06

weaverryan force-pushed the asset-mapper-warn-incompat branch 2 times, most recently from e741d13 to 08b6059 Compare October 18, 2023 13:06

nicolas-grekas reviewed Oct 19, 2023

View reviewed changes

src/Symfony/Component/AssetMapper/ImportMap/ImportMapVersionChecker.php Outdated Show resolved Hide resolved

[AssetMapper] Warn of missing or incompat dependencies

42dfb9a

nicolas-grekas force-pushed the asset-mapper-warn-incompat branch from 08b6059 to 42dfb9a Compare October 19, 2023 15:21

nicolas-grekas merged commit 071a971 into symfony:6.4 Oct 19, 2023

nicolas-grekas mentioned this pull request Oct 19, 2023

[AssetMapper] Warn of missing or incompat dependencies symfony/symfony-docs#19052

Closed

weaverryan deleted the asset-mapper-warn-incompat branch October 19, 2023 18:00

This was referenced Oct 21, 2023

Release v6.4.0-BETA1 #52220

Merged

Release v7.0.0-BETA1 #52224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AssetMapper] Warn of missing or incompat dependencies #51849

[AssetMapper] Warn of missing or incompat dependencies #51849

[AssetMapper] Warn of missing or incompat dependencies #51849

[AssetMapper] Warn of missing or incompat dependencies #51849

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment