8000 [AssetMapper] Always downloading vendor files by weaverryan · Pull Request #51786 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetMapper] Always downloading vendor files #51786

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
Oct 6, 2023
Merged

[AssetMapper] Always downloading vendor files #51786

merged 1 commit into from
Oct 6, 2023

Conversation

weaverryan
Copy link
Member
@weaverryan weaverryan commented Sep 29, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? yes
New feature? yes
Deprecations? no
Tickets None
License MIT

Hi!

I discovered a few problems with our CDN set up:

A) The jsdelivr CDN uses imports statements with specific versions - e.g. import{Controller as t}from"/npm/@hotwired/stimulus@3.2.1/+esm";. This could cause the same dependency to be downloaded (at different verisons) every time.

B) The jspm CDN has correct bare imports - e.g. import '@hotwired/stimulus';. However, to they also include some "in-package" relative imports - e.g.import '../other-file.js'. That is ok on the CDN, but to support local download, it means we ALSO need to download those files. This is possible, but adds quite a bit of complexity and would probably require an importmap.php.local file so we could keep track of all of the files needed for some foo-bar package.

Solution

Talking with @nicolas-grekas, we're going to change AssetMapper to "download-only" - like Composer, npm, etc. The importmap.php file will change to look like:

return [
    // no change for local files
    'app' => [
        'path' => './assets/app.js',
        'entrypoint' => true,
    ],

    // remote packages have only version
    'bootstrap' => [
        'version' => '5.3.2',
    ],

We will exclusively use jsdelivr as the "backend", as it avoids the "in-package" relative imports. So, every package == 1 file.

To download the remote packages, run:

php bin/console importmap:install

This downloads everything into an assets/vendor/ directory (which will be ignored by git).

Other Notable Items

  • Added MappedAsset.isVendor property, which is a simple check to see if the file lives in assets/vendor/. If it does, we still run the compilers, but we can silence invalid import warnings.

  • Mapped assets ending in .php are excluded from AssetMapper. There could be edge-cases where you don't want this, but it felt like a safer default, and allowed me to easily NOT expose the new vendor/assets/installed.php file.

TODO

  • Re-add code to change downloaded contents to bare imports
  • Download vendor packages after update/require
  • In JsCompiler, don't warn on vendor invalid imports
  • Remove package resolver system
  • Fix tests
  • Audit to see what tests are missing

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@weaverryan weaverryan marked this pull request as ready for review September 29, 2023 18:45
@carsonbot carsonbot added this to the 6.4 milestone Sep 29, 2023
@carsonbot carsonbot changed the title [WIP][AssetMapper] Always downloading vendor files [AssetMapper] [WIP] Always downloading vendor files Sep 29, 2023
@weaverryan weaverryan force-pushed the asset-mapper-always-download branch from 42781ed to f9f357c Compare October 2, 2023 00:52
@weaverryan weaverryan changed the title [AssetMapper] [WIP] Always downloading vendor files [AssetMapper] Always downloading vendor files Oct 2, 2023
@weaverryan
Copy link
Member Author

This is ready to go! And important for 6.4. Test failures look unrelated.

@weaverryan weaverryan force-pushed the asset-mapper-always-download branch from eceb490 to 94082d6 Compare October 2, 2023 12:58
@weaverryan weaverryan force-pushed the asset-mapper-always-download branch 2 times, most recently from f26669d to 7722e10 Compare October 3, 2023 13:51
@weaverryan weaverryan mentioned this pull request Oct 3, 2023
Merged
@weaverryan
Copy link
Member Author

Ready to go - test failures are expected (due to FWBundle not using the code from this PR)

@weaverryan weaverryan mentioned this pull request Oct 3, 2023
Merged
@weaverryan weaverryan mentioned this pull request Oct 4, 2023
Merged
@chalasr
Copy link
Member
chalasr commented Oct 5, 2023

Rebase needed

@weaverryan weaverryan force-pushed the asset-mapper-always-download branch from 732c049 to ac14da5 Compare October 5, 2023 14:20
weaverryan
weaverryan commented Oct 5, 2023
View reviewed changes
src/Symfony/Component/AssetMapper/ImportMap/ImportMapManager.php
// https://regex101.com/r/MDz0bN/1
$regex = '/(?:(?P<registry>[^:\n]+):)?((?P<package>@?[^=@\n]+))(?:@(?P<version>[^=\s\n]+))?(?:=(?P<alias>[^\s\n]+))?/';
// https://regex101.com/r/z1nj7P/1
$regex = '/((?P<package>@?[^=@\n]+))(?:@(?P<version>[^=\s\n]+))?(?:=(?P<alias>[^\s\n]+))?/';
Copy link
Member Author

Choose a reason for hiding this comment

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

The registry is a jspm concept, so I'm removing it.

nicolas-grekas
nicolas-grekas reviewed Oct 5, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I love it!

@weaverryan
Copy link
Member Author

Thanks for the check! Changes made

@weaverryan weaverryan mentioned this pull request Oct 5, 2023
Closed
@weaverryan weaverryan force-pushed the asset-mapper-always-download branch from bbacaee to 98e9cd6 Compare October 5, 2023 20:28
@fabpot fabpot force-pushed the asset-mapper-always-download branch from e3a4956 to 18e4200 Compare October 6, 2023 14:13
@fabpot
Copy link
Member
fabpot commented Oct 6, 2023
< 628C div class="edit-comment-hide">

Thank you @weaverryan.

@fabpot fabpot mentioned this pull request Oct 6, 2023
Closed
@fabpot fabpot merged commit 204381b into symfony:6.4 Oct 6, 2023
@weaverryan weaverryan deleted the asset-mapper-always-download branch October 6, 2023 14:39
@maelanleborgne maelanleborgne mentioned this pull request Oct 6, 2023
Merged
3 tasks
fabpot added a commit that referenced this pull request Oct 16, 2023
@fabpot
feature #51828 [AssetMapper] Put importmap in polyfill so it can be h…
7cf6f77 
…osted locally easily (weaverryan)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[AssetMapper] Put importmap in polyfill so it can be hosted locally easily

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #51302
| License       | MIT

Note: Built on top of #51786

In #51302, it was asked to allow the polyfill to be an asset mapper path. We could do that, but it would still require the user to "manage" this vendor file locally - i.e. commit it into their repository. So this PR goes a bit further and requires your polyfill to be an item in `importmap.php`. That's a bit odd, but because of #51786, it allows the JS package to be downloaded like any other JS package. The item is actually removed from the `importmap` before it's finally dumped. So the small oddity of having this item in `importmap.php` makes a really nice DX.

Cheers!

Commits
-------

d2014eb [AssetMapper] Put importmap in polyfill so it can be hosted locally easily
nicolas-grekas added a commit that referenced this pull request Oct 19, 2023
@nicolas-grekas
feature #51849 [AssetMapper] Warn of missing or incompat dependencies…
071a971 
… (weaverryan)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[AssetMapper] Warn of missing or incompat dependencies

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | None
| License       | MIT

Hi!

The aim of the `importmap.php` system is to be a simple way to manage your JS dependencies. But to make it robust enough for production use, it does need a few things - like the `importmap:audit` command in #51650. This PR adds a check, during `importmap:require` and `importmap:update`, that reports any missing dependencies or dependencies with invalid versions. This is necessary so that, if package `A` requires package `B`, their versions don't "drift" over time without you being aware (e.g. you update package `A` to v3 but keep package `B` at v1, even though v3 of `A` requires v2 of `B`).

<img width="1266" alt="Screenshot 2023-10-04 at 2 44 04 PM" src="https://github.com/symfony/symfony/assets/121003/3901a070-d092-494a-a7cb-3bfe5d5a99f9">

Built on top of #51786.

Cheers!

Commits
-------

42dfb9a [AssetMapper] Warn of missing or incompat dependencies
This was referenced Oct 21, 2023
Merged
Merged
@freerich freerich mentioned this pull request Nov 3, 2023
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Nov 13, 2023
@javiereguiluz
minor #19111 [AssetMapper] Remove --download option that has become…
49146be 
… obsolete (freerich)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[AssetMapper] Remove `--download` option that has become obsolete

Minor adjustments corresponding to symfony/symfony#51786

Commits
-------

9f1f17f [AssetMapper] Remove `--download` option that has become obsolete
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

@stof stof stof requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
AssetMapper Bug Feature Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
7 participants
@weaverryan @carsonbot @chalasr @fabpot @nicolas-grekas @stof @smnandre
0