8000 [AssetMapper] Using ?specifier=* does not match unstable packages on jsdelivr by weaverryan · Pull Request #52119 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[AssetMapper] Using ?specifier=* does not match unstable packages on jsdelivr #52119

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

weaverryan
Copy link
Member
Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #52106
License MIT

To find which version of a package to grab, we use this API endpoint - https://data.jsdelivr.com/v1/packages/npm/@tabler/core/resolved. If a version constraint is specified (e.g. importmap:require 'bootstrap@^5'), we add a ?specifier=5. If no version is specified, previously we added ?specifier=*. However, this seems to ignore beta releases (returning null for @table/core, whose only has a 1.0 beta). Omitting ?specifier seems to grab the latest, no matter what it is.

Also, if for some reason, we still can't find a version, we know blow up with a much clearer exception.

Cheers!

@@ -32,9 +32,6 @@ final class JsDelivrEsmResolver implements PackageResolverInterface

public function __construct(
HttpClientInterface $httpClient = null,
private readonly string $versionUrlPattern = self::URL_PATTERN_VERSION,
private readonly string $distUrlPattern = self::URL_PATTERN_DIST,
private readonly string $distUrlCssPattern = self::URL_PATTERN_DIST_CSS
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: but it really doesn't make sense to have these URLs configurable. We are hitting very exact URLs.

@weaverryan weaverryan force-pushed the asset-mapper-fix-jsdelivr-specifier-wildcard branch from 4d6f72e to 370f6dc Compare October 19, 2023 18:11
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 19, 2023
@fabpot
Copy link
Member
fabpot commented Oct 20, 2023

Thank you @weaverryan.

@fabpot fabpot merged commit 97bbd4e into symfony:6.4 Oct 20, 2023
@tacman
Copy link
Contributor
tacman commented Oct 20, 2023

I put this in the PR, but it probably belongs here in the issue instead.

Installing the bootstrap styling of datatables.net is failing for me.

symfony new assetmapper-bug  --webapp --version=next --php=8.2 && cd assetmapper-bug
composer config extra.symfony.allow-contrib true
composer req symfony/asset-mapper:^6.4
bin/console importmap:require datatables.net-bs5

@weaverryan weaverryan deleted the asset-mapper-fix-jsdelivr-specifier-wildcard branch October 20, 2023 11:00
@weaverryan
Copy link
Member Author

I put this in the PR, but it probably belongs here in the issue instead.

I'm on it - thanks :)

fabpot added a commit that referenced this pull request Oct 20, 2023
… import then export are adjacent (weaverryan)

This PR was merged into the 6.4 branch.

Discussion
----------

[AssetMapper] Fixing bug of bad parsing of imports when an import then export are adjacent

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | See #52119 (comment) though this appears to be unrelated to that PR
| License       | MIT

There should be a finite number of import statement formats. `@tacman` found a new one! :)

Cheers!

Commits
-------

e5bd7d0 [AssetMapper] Fixing bug of bad parsing of imports when an import then export are adjacent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AssetMapper Bug ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AssetMapper] Failing installs, add verbose error messages, custom url
6 participants
0