8000 [AssetMapper] [Bugfix] Added packageName and filePath to ImportMapEntry when requiring package by maelanleborgne · Pull Request #51983 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] [Bugfix] Added packageName and filePath to ImportMapEntry when requiring package #51983

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[AssetMapper] [Bugfix] Added packageName and filePath to ImportMapEntry when requiring package #51983

wants to merge 3 commits into from

Conversation

maelanleborgne
Copy link
Contributor
Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT

In #51845, 2 new properties have been added to ImportMapEntry that are not initialized in ImportMapManager when calling importmap:require a_package.
This PR fixes this.

@carsonbot carsonbot added this to the 6.4 milestone Oct 11, 2023
weaverryan
weaverryan requested changes Oct 11, 2023
View reviewed changes
Copy link
Member
@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Could you also update JsDelivrEsmResolver::downloadPackages() to throw an exception before the HTTP request if $entry->packageName or $entry->version is null?

@maelanleborgne
Copy link
Contributor Author

Could you also update JsDelivrEsmResolver::downloadPackages() to throw an exception before the HTTP request if $entry->packageName or $entry->version is null?

@weaverryan I'm having a hard time writing an exception message to describe the problem because I don't see how we could reach the downloadPackages method with an entry that has a null packageName of filePath.
The splitPackageNameAndFilePath method defaults packageName to importName and filePath to '' when creating the ImportMapEntries in ImportMapConfigReader.

@weaverryan
Copy link
Member

Yes, the problem should actually NEVER happen. It's more of coding defensively... in case we make some mistake in the future. So, the error message doesn't need to be super specific. For example:

if (null === $entry->packageName) {
    throw new \InvalidArgumentException(sprintf('Missing packageName for importmap entry "%s"., $entry->importName));
}

@maelanleborgne maelanleborgne force-pushed the hotfix/add-packagename-and-filepath-to-importmapentry branch from fa27af3 to 8adabc1 Compare October 12, 2023 12:28
@maelanleborgne maelanleborgne force-pushed the hotfix/add-packagename-and-filepath-to-importmapentry branch from c28abee to 47e334d Compare October 12, 2023 12:57
@weaverryan
Copy link
Member

I'm going to close this - but thanks for getting it going @maelanleborgne. Based on our conversation here, I could see how important it was to go and fix my ugly "packageName + filePath" everywhere. I've done that now on #52024

Thanks!

@weaverryan weaverryan closed this Oct 12, 2023
@maelanleborgne maelanleborgne deleted the hotfix/add-packagename-and-filepath-to-importmapentry branch March 14, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan requested changes

Assignees
No one assigned
Labels
AssetMapper Bug Status: Needs Work
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
3 participants
@maelanleborgne @weaverryan @carsonbot
0