Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[WIP] [AssetMapper] Allow Asset Mapper to use other package resolvers#54133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedMar 1, 2024
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
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 ? |
| @@ -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'), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I will check the Notifier Transports implementation then.
I exposed it here as I didn't have found another way to allow theImportMapManager to pick the right resolver depending on the package 😅
| @@ -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') | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This,imho cannot offer a predictable DX as importmap:install / importmap:require / importmap:update (etc..) would then be inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 editingimportmap.php
| { | ||
| return new self($importName, $importMapType, $path, $isEntrypoint, $version, $packageModuleSpecifier); | ||
| return new self($importName, $importMapType, $path, $isEntrypoint, $resolver, $version, $packageModuleSpecifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This would break BC no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| @@ -27,7 +27,7 @@ public function __construct( | |||
| private readonly AssetMapperInterface $assetMapper, | |||
| private readonly ImportMapConfigReader $importMapConfigReader, | |||
| private readonly RemotePackageDownloader $packageDownloader, | |||
| private readonlyPackageResolverInterface $resolver, | |||
| private readonlyPackageResolverRegistry $resolverRegistry, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Cannot do this without breaking BC i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to have a little explanation here then 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that's a BC break indeed for people that use the class directly without the FWB integration
As for now, there won't be any problem during update as the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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. Seehttp://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.
| @@ -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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
| @@ -26,6 +26,7 @@ private function __construct( | |||
| */ | |||
| public readonly string $path, | |||
| public readonly bool $isEntrypoint, | |||
| public readonly ?string $resolverAlias, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
adding an new required argument is a BC break
| @@ -27,7 +27,7 @@ public function __construct( | |||
| private readonly AssetMapperInterface $assetMapper, | |||
| private readonly ImportMapConfigReader $importMapConfigReader, | |||
| private readonly RemotePackageDownloader $packageDownloader, | |||
| private readonlyPackageResolverInterface $resolver, | |||
| private readonlyPackageResolverRegistry $resolverRegistry, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that's a BC break indeed for people that use the class directly without the FWB integration
| @@ -112,6 +112,7 @@ private function updateImportMapConfig(bool $update, array $packagesToRequire, a | |||
| $entry->packageModuleSpecifier, | |||
| null, | |||
| $importName, | |||
| resolver: $entry->resolverAlias, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no named args if not needed please
| /** | ||
| * Returns the alias of the resolver. | ||
| */ | ||
| public function getAlias(): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
adding a method to an interface is a BC break also
AienTech commentedAug 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Any plans to release this feature? or any work around suggestions till it is officially releases (I've already gave 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 */thrownewError('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? |
This PR allows to provide a specific package resolver when running the
importmap:requirecommand.This is useful when the required package cannot be built or found with the default package resolver (JsDeliver).
It adds a
resolveroption 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.resolverservice to a custom resolver implementing\Symfony\Component\AssetMapper\ImportMap\Resolver\PackageResolverInterface.So the command will look like :
bin/console importmap:require some-package --resolver=resolverFor now only the base implementation is added. I'll provide one or two package resolvers as example