[WIP] [AssetMapper] Allow Asset Mapper to use other package resolvers #54133

adeys · 2024-03-01T19:03:05Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	yes
Issues	Fix #53999
License	MIT

This PR allows to provide a specific package resolver when running the importmap:require command.
This is useful when the required package cannot be built or found with the default package resolver (JsDeliver).

It adds a resolver option to the command that resolves to the default package resolver when none is provided.
The default package resolver can also be changed by mapping the asset_mapper.importmap.resolver service to a custom resolver implementing \Symfony\Component\AssetMapper\ImportMap\Resolver\PackageResolverInterface.
So the command will look like :
bin/console importmap:require some-package --resolver=resolver

For now only the base implementation is added. I'll provide one or two package resolvers as example

submit changes to the documentation
add more resolvers (unpkg ? jspm ?)

…ing require or update processes

…mmand

carsonbot · 2024-03-01T19:03:11Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

…ommandTest`

smnandre · 2024-03-01T23:51:47Z

How would you handle updates ? If you had to select a specific resolver to require a module, i guess the update won't go well unless using the same resolver ?

smnandre · 2024-03-01T23:53:29Z

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

@@ -167,7 +168,7 @@
                service('asset_mapper'),
                service('asset_mapper.importmap.config_reader'),
                service('asset_mapper.importmap.remote_package_downloader'),
-                service('asset_mapper.importmap.resolver'),
+                service('asset_mapper.importmap.resolver_registry'),


I'd suggest using a strategy/facade resolver with the same interface.. i don't think this registry should be exposed.. Maybe it would be better to mimic Notifier Transports class ?

I will check the Notifier Transports implementation then.

I exposed it here as I didn't have found another way to allow the ImportMapManager to pick the right resolver depending on the package 😅

smnandre · 2024-03-01T23:55:50Z

src/Symfony/Component/AssetMapper/Command/ImportMapRequireCommand.php

@@ -44,6 +44,7 @@ protected function configure(): void
            ->addArgument('packages', InputArgument::IS_ARRAY | InputArgument::REQUIRED, 'The packages to add')
            ->addOption('entrypoint', null, InputOption::VALUE_NONE, 'Make the package(s) an entrypoint?')
            ->addOption('path', null, InputOption::VALUE_REQUIRED, 'The local path where the package lives relative to the project root')
+            ->addOption('resolver', null, InputOption::VALUE_REQUIRED, 'The alias of the package resolver to use')


This, imho cannot offer a predictable DX as importmap:install / importmap:require / importmap:update (etc..) would then be inconsistent.

I think it won't have an impact as the resolver config is added to the importmap file.
During (importmap:install / importmap:update) commands, if a custom resolver was provided, it will be used, otherwise the default one (jsdeliver) will be used.
It will just be a matter of editing importmap.php

smnandre · 2024-03-01T23:59:50Z

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

    {
-        return new self($importName, $importMapType, $path, $isEntrypoint, $version, $packageModuleSpecifier);
+        return new self($importName, $importMapType, $path, $isEntrypoint, $resolver, $version, $packageModuleSpecifier);


This would break BC no ?

IMHO no, as a default resolver is provided in the asset mapper config
As this part is not exposed (I think), it will just be resolved internally

smnandre · 2024-03-02T00:00:51Z

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

@@ -27,7 +27,7 @@ public function __construct(
        private readonly AssetMapperInterface $assetMapper,
        private readonly ImportMapConfigReader $importMapConfigReader,
        private readonly RemotePackageDownloader $packageDownloader,
-        private readonly PackageResolverInterface $resolver,
+        private readonly PackageResolverRegistry $resolverRegistry,


Cannot do this without breaking BC i think

I'd like to have a little explanation here then 😅

that's a BC break indeed for people that use the class directly without the FWB integration

adeys · 2024-03-02T00:29:25Z

How would you handle updates ? If you had to select a specific resolver to require a module, i guess the update won't go well unless using the same resolver ?

As for now, there won't be any problem during update as the ImportMapManager can pick the right resolver to check and perform the update for each package. Even if the resolver was changed, the default one will be used to get fresh package files as the old files are removed first in the upgrade process

nicolas-grekas

Sorry, the current BC layer (or lack of) needs to be improved to be up to our standards.
Think that the component can be used standalone. See http://symfony.com/bc for what can and cannot be done. For most changes, we do have patterns to create working BC layers. Branch 6.4 contains a lot if you need inspiration. We're here to help also of course.

nicolas-grekas · 2024-03-18T14:20:36Z

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

@@ -127,11 +133,11 @@ public function findRootImportMapEntry(string $moduleName): ?ImportMapEntry
        return $entries->has($moduleName) ? $entries->get($moduleName) : null;
    }

-    public function createRemoteEntry(string $importName, ImportMapType $type, string $version, string $packageModuleSpecifier, bool $isEntrypoint): ImportMapEntry
+    public function createRemoteEntry(string $importName, ImportMapType $type, string $version, string $packageModuleSpecifier, bool $isEntrypoint, ?string $resolver): ImportMapEntry


adding an argument is a BC break so we need to either find another approach or do a proper BC layer (using @param + func_get_arg, see branch 6.4 for inspiration)

nicolas-grekas · 2024-03-18T14:20:56Z

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

@@ -26,6 +26,7 @@ private function __construct(
         */
        public readonly string $path,
        public readonly bool $isEntrypoint,
+        public readonly ?string $resolverAlias,


adding an new required argument is a BC break

nicolas-grekas · 2024-03-18T14:22:03Z

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

@@ -27,7 +27,7 @@ public function __construct(
        private readonly AssetMapperInterface $assetMapper,
        private readonly ImportMapConfigReader $importMapConfigReader,
        private readonly RemotePackageDownloader $packageDownloader,
-        private readonly PackageResolverInterface $resolver,
+        private readonly PackageResolverRegistry $resolverRegistry,


that's a BC break indeed for people that use the class directly without the FWB integration

nicolas-grekas · 2024-03-18T14:22:44Z

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

@@ -112,6 +112,7 @@ private function updateImportMapConfig(bool $update, array $packagesToRequire, a
                    $entry->packageModuleSpecifier,
                    null,
                    $importName,
+                    resolver: $entry->resolverAlias,


no named args if not needed please

nicolas-grekas · 2024-03-18T14:23:07Z

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

+    /**
+     * Returns the alias of the resolver.
+     */
+    public function getAlias(): string;


adding a method to an interface is a BC break also

AienTech · 2024-08-26T10:39:50Z

Any plans to release this feature? or any work around suggestions till it is officially releases (I've already gave "symfony/asset-mapper": "7.2.x-dev" a shot)?

jsdelivr is causing some issues recently:

/**
 * Failed to bundle using Rollup v2.79.1: failed to resolve an internal import.
 * If you believe this to be an issue with jsDelivr, and not with the package itself, please open an issue at https://github.com/jsdelivr/jsdelivr
 */

 throw new Error('Failed to bundle using Rollup v2.79.1: failed to resolve an internal import. If you believe this to be an issue with jsDelivr, and not with the package itself, please open an issue at https://github.com/jsdelivr/jsdelivr');

and just out of curiosity, why not getting the packages directly from npmjs.org repos?

adeys added 4 commits March 1, 2024 19:47

[AssetMapper] Add package resolver registry

828519b

[AssetMapper] Add resolver alias name to importmap.php

2856b89

[AssetMapper] Allow ImportMapManager to pick the right resolver dur…

905714e

…ing require or update processes

[AssetMapper] Add resolver option help text to importmap:require co…

c9e6e12

…mmand

carsonbot added Status: Needs Review AssetMapper Deprecation Feature labels Mar 1, 2024

carsonbot added this to the 7.1 milestone Mar 1, 2024

adeys added 2 commits March 1, 2024 20:16

[AssetMapper] Fix failing tests

6216808

[AssetMapper] Fix required TestCase namespace in `ImportMapRequireC…

f81e407

…ommandTest`

adeys changed the title ~~[AssetMapper] Allow Asset Mapper to use other package resolvers~~ [WIP] [AssetMapper] Allow Asset Mapper to use other package resolvers Mar 1, 2024

smnandre reviewed Mar 1, 2024

View reviewed changes

smnandre reviewed Mar 2, 2024

View reviewed changes

Chris53897 mentioned this pull request Mar 6, 2024

Add documentation for AssetMapper installation tattali/CalendarBundle#70

Open

nicolas-grekas reviewed Mar 18, 2024

View reviewed changes

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

Uh oh!

[WIP] [AssetMapper] Allow Asset Mapper to use other package resolvers #54133

Are you sure you want to change the base?

[WIP] [AssetMapper] Allow Asset Mapper to use other package resolvers #54133

Uh oh!

Conversation

adeys commented Mar 1, 2024

Uh oh!

carsonbot commented Mar 1, 2024

Uh oh!

smnandre commented Mar 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adeys commented Mar 2, 2024

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AienTech commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AienTech commented Aug 26, 2024 •

edited

Loading