8000 [AssetMapper] Add `--dry-run` option on `importmap:require` command by chadyred · Pull Request #59464 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] Add --dry-run option on importmap:require command #59464

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
Merged
merged 1 commit into from
Mar 10, 2025
Merged

[AssetMapper] Add --dry-run option on importmap:require command #59464

merged 1 commit into from
Mar 10, 2025

Conversation

@chadyred
Copy link
Contributor
@chadyred chadyred commented Jan 10, 2025 •
edited
Loading
Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT

We are working with @smnandre on several ideas to improve AssetMapper DX, focusing here on the importmap:require command.

Currently, there is no way to know in advance which dependent packages will be downloaded too, and the final output display lacks clarity.

This PR aims to solve both issues by adding a new --dry-run flag and reworking the command output (with -v verbosity)

1
Console output 
php bin/console importmap:require bootstrap --dry-run -v

[DRY-RUN] No changes will apply to the importmap configuration.

 -------------------------------------- --------- ---------------------------------------------------- 
  Package                                Version   Path                                                
 -------------------------------------- --------- ---------------------------------------------------- 
  bootstrap                              5.3.3     assets/vendor/bootstrap/bootstrap.index.js          
  @popperjs/core                         2.11.8    assets/vendor/@popperjs/core/core.index.js          
  bootstrap/dist/css/bootstrap.min.css   5.3.3     assets/vendor/bootstrap/dist/css/bootstrap.min.css  
 -------------------------------------- --------- ---------------------------------------------------- 

                                                                                                                        
 [OK] 3 new items (bootstrap, @popperjs/core, bootstrap/dist/css/bootstrap.min.css) added to the importmap.php!         
                                                                                                                        

[DRY-RUN] No changes applied to the importmap configuration.

New --dry-run option

This flag allows developers to simulate the installation process, without any alteration:

  • no modification of importmap.php file;
  • no file added or updated in the assets and public directories.

The --dry-run flag allows developers to:

  • check commands for mistakes (e.g., typing bootrap instead of bootstrap, or jquery);
  • list dependent packages that will be installed (e.g., requiring bootstrap will also install @popperjs/core);
  • test for potential issues while downloading1 files.

The console will display a line at start and end of execution to signal the dry-run mode.

2
Console output 
php bin/console importmap:require bootstrap --dry-run

[DRY-RUN] No changes will apply to the importmap configuration.

                                                                                                                        
 [OK] 3 new items (bootstrap, @popperjs/core, bootstrap/dist/css/bootstrap.min.css) added to the importmap.php!         
                                                                                                                        

[DRY-RUN] No changes applied to the importmap configuration.

New -v verbose output

This PR also reworks the output for the -v (verbose) flag to provide more clarity when running the command with additional verbosity.

3

All feedback welcome!

Footnotes

  1. AssetMapper recursively extracts the dependencies of JavaScript modules. Even with the --dry-run option, the importmap:require command requires a working HttpClient to download metadata and JavaScript files from various registries/repositories (mainly JSDelivr for now). ↩

@carsonbot carsonbot added this to the 7.3 milestone Jan 10, 2025
@chadyred chadyred force-pushed the feat/command-importmap-require-details-2 branch from b4ec116 to ed6a4d2 Compare January 10, 2025 13:28
chalasr
chalasr requested changes Jan 21, 2025
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Please add an entry in the UPGRADE-7.3 file also

@chadyred chadyred force-pushed the feat/command-importmap-require-details-2 branch from ed6a4d2 to 97354d5 Compare January 27, 2025 08:46
stof
stof reviewed Jan 27, 2025
View reviewed changes
@chadyred chadyred force-pushed the feat/command-importmap-require-details-2 branch from 467ac15 to 4ca59ff Compare January 27, 2025 11:00
@chadyred
Copy link
Contributor Author
chadyred commented Jan 27, 2025

Status: Needs Review

@chadyred chadyred force-pushed the feat/command-importmap-require-details-2 branch 3 times, most recently from 4062322 to e834020 Compare February 3, 2025 14:48
@OskarStark
Copy link
Contributor
OskarStark commented Feb 3, 2025

I cannot see the images in the PR header

@chadyred
Copy link
Contributor < 10BC0 /span> Author
chadyred commented Feb 3, 2025 •
edited
Loading

@OskarStark thanks, it is ok now, it was a copy / paste from a private repository. I take note about that and the images ^^'

@smnandre
Copy link
Member
smnandre commented Feb 3, 2025

I cannot see the images in the PR header

@OskarStark

capture

@chadyred chadyred force-pushed the feat/command-importmap-require-details-2 branch from e834020 to 4ce815f Compare February 4, 2025 14:14
@chadyred chadyred force-pushed the feat/command-importmap-require-details-2 branch from 4ce815f to 8537fff Compare February 20, 2025 12:11
@smnandre
Copy link
Member
smnandre commented Feb 22, 2025

Anything here to be added @chalasr ?

@chadyred chadyred requested a review from chalasr February 27, 2025 08:26
fabpot
fabpot requested changes Mar 3, 2025
View reviewed changes
Copy link
Member
@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM

@chadyred chadyred force-pushed the feat/command-importmap-require-details-2 branch from 8537fff to 337c465 Compare March 7, 2025 07:36
@chadyred chadyred requested a review from fabpot March 10, 2025 15:23
@fabpot
Copy link
Member
fabpot commented Mar 10, 2025

Thank you @chadyred.

@fabpot fabpot merged commit c11abf2 into symfony:7.3 Mar 10, 2025
10 of 11 checks passed
@fabpot fabpot mentioned this pull request Mar 10, 2025
Closed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@chalasr chalasr Awaiting requested review from chalasr

Assignees

No one assigned

Labels

AssetMapper Feature Status: Reviewed

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

7 participants

@chadyred @OskarStark @smnandre @fabpot @stof @chalasr @carsonbot
0