8000 [AssetMapper] Add outdated command by maelanleborgne · Pull Request #51845 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] Add outdated command #51845

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2023
Merged

[AssetMapper] Add outdated command #51845

merged 1 commit into from
Oct 10, 2023

Conversation

maelanleborgne
Copy link
Contributor
@maelanleborgne maelanleborgne commented Oct 4, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT

As suggested by @WebMamba, I added a new command for the AssetMapper component to list outdated 3rd party packages. It is inspired by the composer outdated command so I tried to replicate its options as much as I could.
It reads the importmap.php and extract packages version to query the https://registry.npmjs.org/%package% API and read the latest version from metadata.

image

⚠️ The code is base on #51650 branch so it is not ready to be merged yet, but I'd be happy to get some reviews and feedback in the meantime. This is my first PR on symfony, so there will probably be a lot to say !

@carsonbot carsonbot added this to the 6.4 milestone Oct 4, 2023
@carsonbot
Copy link

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 6.4 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

@carsonbot carsonbot changed the title [WIP] [AssetMapper] Add outdated command [AssetMapper] [WIP] Add outdated command Oct 4, 2023
@maelanleborgne maelanleborgne force-pushed the feature/assetmapper-outdated branch from 9f64774 to 048633c Compare October 4, 2023 15:19
weaverryan
weaverryan reviewed Oct 4, 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.

Congrats on your first PR! This is really nice work - I've left some comments below to get things started. FYI, you don't need to (you shouldn't) click to merge my suggestions. Instead, "steal" them and commit them yourself. That will keep your commits clean and all authored by you.

Thanks!

@maelanleborgne
Copy link
Contributor Author

Thank you very much for the review and the feedback @weaverryan 🙏 . I've made changes according to your suggestions, let me know if you see anything else I could do.

@maelanleborgne maelanleborgne force-pushed the feature/assetmapper-outdated branch from fe0882a to 2a7d020 Compare October 5, 2023 09:12
@weaverryan
Copy link
Member

@maelanleborgne #51650 is merged, so you can rebase your commits to remove them from this PR. Probably THAT PR was also rebased, so this can get a little tricky - I've been doing it a lot lately 😆

@maelanleborgne maelanleborgne force-pushed the feature/assetmapper-outdated branch from 2a7d020 to 9da7241 Compare October 5, 2023 12:43
weaverryan
weaverryan reviewed Oct 5, 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.

Looking good 👍 - nice job rebasing - next round of comments

smnandre
smnandre reviewed Oct 5, 2023
View reviewed changes
src/Symfony/Component/AssetMapper/Command/ImportMapOutdatedCommand.php
{
$io = new SymfonyStyle($input, $output);
$packages = $input->getArgument('packages');
$packagesUpdateInfos = $this->updateChecker->getAvailableUpdates($packages);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be used in the update command too ?

Copy link
Member
8000

Choose a reason for hiding this comment

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

Can you explain more? Do you mean: in the update command, we would first use this function to determine which updates are possible and then loop over and make them? If so, that's a different way than we're doing it now - but I can't think of any big difference in final behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Yes that was the idea, but i'm not talking about "right now" ... but in a near future, as we already said :)

@weaverryan
Copy link
Member

One more relevant PR just got merged- can you rebase? Let me know if you have any questions - hopefully that’ll simplify things, as remote packages ALWAYS have a version now.

@weaverryan weaverryan added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 6, 2023
@OskarStark OskarStark changed the title [AssetMapper] [WIP] Add outdated command [AssetMapper] [WIP] Add outdated command Oct 6, 2023
@OskarStark
Copy link
Contributor

Is this still WIP?

@maelanleborgne
Copy link
Contributor Author

Is this still WIP?

Yes I still have some work to do now that #51786 has been merged.

@maelanleborgne maelanleborgne force-pushed the feature/assetmapper-outdated branch from 9da7241 to fd738e1 Compare October 9, 2023 07:12
weaverryan
weaverryan reviewed Oct 9, 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.

A few minor, but important tweaks. Also, don't forget to check fabbot in the CI after you push for the little phpcs details :).

@maelanleborgne maelanleborgne mentioned this pull request Oct 9, 2023
Merged
weaverryan
weaverryan reviewed Oct 10, 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.

Ok, very close now!

@maelanleborgne maelanleborgne force-pushed the feature/assetmapper-outdated branch from 4414448 to 6d3ad99 Compare October 10, 2023 12:49
weaverryan
weaverryan approved these changes Oct 10, 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.

Test failures all look unrelated. 👍

squrious
squrious reviewed Oct 10, 2023
View reviewed changes
src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapUpdateCheckerTest.php Outdated
version: '5.3.1',
packageName: 'bootstrap',
F438 ),
// Css will always be considered up to date
Copy link
Contributor

Choose a reason for hiding this comment

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

Why CSS should be always considered up-to-date? At L95 it's expected to have updateType: 'patch'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, that's a comment that should have been deleted when I made the change to allow css to be managed. Thanks

@weaverryan weaverryan changed the title [AssetMapper] [WIP] Add outdated command [AssetMapper] Add outdated command Oct 10, 2023
@fabpot fabpot force-pushed the feature/assetmapper-outdated branch from a374dde to 4d32a35 Compare October 10, 2023 18:57
@fabpot
Copy link
Member
fabpot commented Oct 10, 2023

Thank you @maelanleborgne.

@fabpot fabpot mentioned this pull request Oct 10, 2023
Closed
@fabpot fabpot merged commit fc12885 into symfony:6.4 Oct 10, 2023
@weaverryan
Copy link
Member
weaverryan commented Oct 11, 2023 •
edited
Loading

Congrats and thanks for the nice work @maelanleborgne! No expectations, but if you'd like to work on a PR to find & change the ->importName spots that should be updated, I'd appreciated it - one less thing on my list :)

Edit: I found one issue. There are 3 places in ImportMapManager (that I forgot about) where we create new ImportMapEntry objects... and some or all of these need to have the packageName and filePath stuff added to it. Currently, because this is missing, importmap:require some-package fails (because eventually an ImportMapEntry is passed to JsDelivrEsmResolver::downloadPackages(), which is missing $entry->packageName, so the URL fails to build correctly there.

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 11, 2023
@javiereguiluz
minor #19005 [AssetMapper] [WCM] Added instruction for 'outdated' com…
ebc7593 
…mand (Maelan LE BORGNE)

This PR was merged into the 6.4 branch.

Discussion
----------

[AssetMapper] [WCM] Added instruction for 'outdated' command

Documentation for the command added in this PR : symfony/symfony#51845

Commits
-------

ef85a01 [AssetMapper] Added instruction for 'outdated' command
@maelanleborgne
Copy link
Contributor Author
maelanleborgne commented Oct 11, 2023 •
edited
Loading

Edit: I found one issue. There are 3 places in ImportMapManager (that I forgot about) where we create new ImportMapEntry objects... and some or all of these need to have the packageName and filePath stuff added to it. Currently, because this is missing, importmap:require some-package fails (because eventually an ImportMapEntry is passed to JsDelivrEsmResolver::downloadPackages(), which is missing $entry->packageName, so the URL fails to build correctly there.

@weaverryan I've checked and it seems like we need to make a change only on this occurence :

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

Line 299 in fc12885

$newEntry = new ImportMapEntry(

The other two are local assets that should not cause trouble. Should I open a PR for the fix or is there an already opened one that we could fit this in ?

@maelanleborgne maelanleborgne mentioned this pull request Oct 11, 2023
Closed
This was referenced Oct 21, 2023
Merged
Merged
@maelanleborgne maelanleborgne deleted the feature/assetmapper-outdated 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

@smnandre smnandre smnandre left review comments

@squrious squrious squrious left review comments

@fabpot fabpot fabpot approved these changes

@weaverryan weaverryan weaverryan approved these changes

Assignees
No one assigned
Labels
AssetMapper Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
7 participants
@maelanleborgne @carsonbot @weaverryan @OskarStark @fabpot @smnandre @squrious
0