Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[AssetMapper] Add support for CSS files in the importmap#51543

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@weaverryan
Copy link
Member

@weaverryanweaverryan commentedSep 4, 2023•
edited
Loading

QA
Branch?6.4
Bug fix?fixes#51417 (fixed this along the way)
New feature?yes
Deprecations?no
TicketsNone
LicenseMIT
Doc PRTODO - but important!

Hi!

This is the brainchild of@nicolas-grekas & I (mostly him). This PR adds 2 big user-facing features:

A) CSS Handling including ability to import CSS

Theimportmap.php now supports a'type' => 'css':

return ['app.css' => ['path' =>'styles/app.css','type' =>'css',    ],

This, by itself, won't cause the CSS file to be loaded. But it WILL be added to the importmap, though the exact behavior will depend on the entrypoint (see next section).

But, in the simplest case, it will output something like this, which adds the<link rel="stylesheet"> tag to the page if this file is every imported in JSimport 'app.css'.

<scripttype="importmap">{"imports":{"app.css":"data:application/javascript,const%20d%3Ddocument%2Cl%3Dd.createElement%28%22link%22%29%3Bl.href%3D%22%2Fassets%2Fstyles%2Fapp-f86cc618674db9b39b179e440063aab4.css%22%2Cl.rel%3D%22stylesheet%22%2C%28d.head%7C%7Cd.getElementsByTagName%28%22head%22%29%5B0%5D%29.appendChild%28l%29"}}</script>

More commonly, in the same way that AssetMapper finds relative JavaScript imports and adds them to the importmap, it also finds relative CSS imports and addsthose to the importmap. This allows you to:import './styles/foo.css' without needing to addfoo.css to the importmap. It "just works". This would result infoo.css being added to the page via JavaScript unless it is in the import chain of an entrypoint (see next section), in which case it would be a true<link> tag.

Also, you can now download CSS files viaimportmap:require (or, if a package comes with a CSS file, it will be downloaded and added automatically - i.e. if the package has astyle key):

# will download `bootstrap` AND `bootstrap/dist/css/bootstrap.min.css`php bin/console importmap:require bootstrap

B) Auto-preload based on entrypoints

Like with Webpack, there is now a concept of "entrypoints". The ONE time you call{{ importmap() }} on the page, you will pass the 1 or many "entrypoint" names fromimportmap.php that should be loaded - e.g.importmap('app') (the most common) orimportmap(['app', 'checkout']).

Most simply (and this is true already in AssetMapper), this causes a<script type="module">import 'app';</script> to be rendered into the page.

But in this PR, it also has another critical role:

  • For each entrypoint, all of the non-lazy JavaScript dependencies are found. So ifapp.js importsother.js importsyet-another.js importssome_styles.css, using non-lazy imports (i.e.import './other.js vs the lazyimport('./other.js')), thenother.js,yet-another.js andsome_styles.css are all returned.
  • For all of these dependencies, they are "preloaded"
    • other.js -> preloaded (i.e.modulepreload tag rendered)
    • yet-another.js -> preloaded (i.e.modulepreload tag rendered)
    • some_styles.css "preloaded" - i.e. a ` is added to the page.

The idea is that, ifapp.js is your entrypoint, thenevery non-lazy import in its import chain will need to be downloaded before the JavaScript starts executing. So all files should be preloaded. Additionally, if we find any CSS that is imported in a non-lazy way, we render those aslink tags.

Thepreload option inimportmap.php is GONE. Preloading is controlled entirely through the entrypoint.

This entrypoint logic also affects the ordering of the non-lazy CSS files (i.e. the CSS files that will be rendered aslink tags). It finds all (in order) non-lazy imported CSS files from the entrypoint and render them aslink tags in order (like Webpack).

I propose the recipe startingimportmap.php is updated to be:

return ['app' => ['path' =>'app.js','entrypoint' =>true,    ],];

And then inassets/app.js:

import'./styles/app.css';

C) Other Improvements

// both will correctly be ignored// import 'foo';/* import 'foo'; */

TODOs

  • Update the 6.4 docs
  • update the 6.4 recipe

nicolas-grekas, Kocal, smnandre, ahmedyakoubi, andreybolonin, welcoMattic, Chris53897, Spomky, yceruto, dunglas, and raphael-geffroy reacted with heart emoji
@weaverryanweaverryanforce-pushed theasset-mapper-css-importmaps branch 2 times, most recently fromfa22633 to5ef0727CompareSeptember 13, 2023 12:35
Copy link
Member

@nicolas-grekasnicolas-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 played with this, it just works :)

When using--download, this will break assets referenced byurl() /@import (exceptdata: ones of course). To fix this, we'd need to parse those and fetch them, recursively.

Also I'm wondering: currently, the importmap recipe adds a link tag. Could we leverage this new feat to not do this in the recipe anymore? Would be nice :)

@weaverryan
Copy link
MemberAuthor

When using --download, this will break assets referenced by url() /@import (except data: ones of course). To fix this, we'd need to parse those and fetch them, recursively.

I'll check into that - we already do something similar for CSS.

Also I'm wondering: currently, the importmap recipe adds a link tag. Could we leverage this new feat to not do this in the recipe anymore? Would be nice :)

Right now, I believe thatlink tag will be removed. Instead, we'll either start the user'simportmap.php with anapp.css entry. Or (what I'm leaning to now) is exactly like Encore: inassets/app.js, you'dimport './styles/app.css'. That would trigger the link tag.

nicolas-grekas reacted with heart emoji

@stof
Copy link
Member

Right now, I believe thatlink tag will be removed. Instead, we'll either start the user'simportmap.php with anapp.css entry. Or (what I'm leaning to now) is exactly like Encore: inassets/app.js, you'dimport './styles/app.css'. That would trigger the link tag.

Please don't.
Until now, the asset-mapper component is about building on web standards. Thisimport './styles/app.css' in JS file is a totally non-standard feature added by webpack.

Kocal reacted with thumbs up emoji

@weaverryan
Copy link
MemberAuthor

Until now, the asset-mapper component is about building on web standards. This import './styles/app.css' in JS file is a totally non-standard feature added by webpack.

That's 100% true. But it's also massively user-friendly. To be precise, there are 2 different sides to this:

A) The ability to have'type' => 'css' inimportmap.php - super nice because you canimportmap:require CSS files instead of needing to managelink tags manually or downloading vendor CSS if you want to avoid a CDN.

B) The ability toimport './some.css'. Totally non-standard, totally optional, but very convenient. This can be used in lazy Stimulus controllers to import CSS only when it's needed.

If/when web standards catch up, we can migrate to whatever that standard is. As a compromise, we could putapp.css inside ofimportmap.php instead of importing it by default inapp.js.importmap.php is already a combination of "importmap standard" + "Symfony additions". Users would still be able to import CSS files from their JavaScript files. But if we set up their mainapp.css file inimportmap.php, then we could better encourage import CSS only when needed.

@stof
Copy link
Member

Having the JS being the trigger to load the CSS is very bad from a performance point of view: the CSS must be render-blocking while the recommended setup is to run the JS code asdefer (so non-blocking).
Relying on an import in the JS to load the CSS through the importmap hack that makesimport 'app.css' load a JS script adding a link tag would lead to a huge FOUC on page load because you would render the page with no CSS at all.

@weaverryan
Copy link
MemberAuthor

Having the JS being the trigger to load the CSS is very bad from a performance point of view: the CSS must be render-blocking while the recommended setup is to run the JS code as defer (so non-blocking).
Relying on an import in the JS to load the CSS through the importmap hack that makes import 'app.css' load a JS script adding a link tag would lead to a huge FOUC on page load because you would render the page with no CSS at all.

You're totally right. Fortunately, that behavior will only be for non-preloaded CSS. The basic idea is:

  • A) If a CSS file ispreload => true, then an actual<link rel="stylesheet"> is added. An importmap entry is also added (to avoid an error in case you DO import the CSS from JS), but it's a noop.

  • B) If a CSS file ispreload => false, then it uses the importmap trick that you mentioned. This is the case where you have some lazily-loaded JS file that imports the CSS file.

Btw, a CSS file can bepreload => true either because it is directly inimportmap.php withpreload: true, or it is non-lazily imported by an entrypoint on the page (this entrypoint idea is still on my machine locally only, I'm finishing some tests).

@weaverryan
Copy link
MemberAuthor

I need to fix a few tests, but PR is ready for review & description has been updated. This is now a robust system for handling CSS and it's fun to use :).

@weaverryan
Copy link
MemberAuthor

Test failures are unrelated or expected (due to the high deps grabbing 7.0 code that doesn't contain the code from the PR).

This is ready for review! You can see some example usage athttps://github.com/weaverryan/ux/tree/assetmapper-css/ux.symfony.com

@weaverryan
Copy link
MemberAuthor

weaverryan commentedSep 27, 2023•
edited
Loading

FYI - tracking down some last details on this...

@weaverryanweaverryanforce-pushed theasset-mapper-css-importmaps branch from467dea6 to2020644CompareSeptember 28, 2023 00:48
@weaverryan
Copy link
MemberAuthor

Found / fixed a problem with cascading the preload into "remote" importmap entries. This is ready again.

@weaverryanweaverryanforce-pushed theasset-mapper-css-importmaps branch froma3d7e1c toe236cbaCompareSeptember 28, 2023 14:56
@weaverryanweaverryanforce-pushed theasset-mapper-css-importmaps branch 2 times, most recently from93a92eb to9a00f0eCompareSeptember 28, 2023 17:17
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM, I just found minor things before approving 🙏

Copy link
Member

@nicolas-grekasnicolas-grekas 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 a line to the changelog (of the twig-bridge also I guess)

weaverryan reacted with thumbs up emoji
}

publicfunctionimportmap(?string$entryPoint ='app',array$attributes = []):string
publicfunctionimportmap(string|array$entryPoint,array$attributes = []):string

Choose a reason for hiding this comment

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

this breaks existing apps that usethe default recipe

is this desired?

Copy link
Member

Choose a reason for hiding this comment

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

Removing support for the implementapp entrypoint should go through a deprecation even if the component is experimental IMO due to this.

Copy link
Member

Choose a reason for hiding this comment

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

and probably also the support of passingnull then if we have a deprecation layer.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

good catch. Check the last commit. I've leftapp as the default (in part for BC). But in the recipe, I WOULD like to start people withimportmap('app') because it's more explicit.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(merging to unlock follow up PRs on the topic, let's iterate. Please report back if you have any concerns of course)

@nicolas-grekas
Copy link
Member

Thank you@weaverryan.

weaverryan reacted with heart emoji

@nicolas-grekasnicolas-grekas merged commit4f6f404 intosymfony:6.4Sep 29, 2023
@weaverryanweaverryan deleted the asset-mapper-css-importmaps branchSeptember 29, 2023 17:18
nicolas-grekas added a commit that referenced this pull requestSep 29, 2023
…p.php (weaverryan)This PR was squashed before being merged into the 6.4 branch.Discussion----------[AssetMapper] Allow simple, relative paths in importmap.php| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | None| License       | MIT| Doc PR        | TODOThis PR is based on top of#51543 - so that needs to be merged first.Currently, the `path` key in `importmap.php` MUST be the logic path to an file. Especially when trying to refer to assets in a bundle, this forces us to use paths that... don't look very obvious to users - e.g.```php    'app' => [        'path' => 'app.js',    ],    '`@symfony`/stimulus-bundle' => [        'path' => '`@symfony`/stimulus-bundle/loader.js',    ],```This PR adds support for relative paths (starting with `.`). This means the `importmap.php` file can look like this now:```php    'app' => [        'path' => './assets/app.js',    ],    '`@symfony`/stimulus-bundle' => [        'path' => './vendor/symfony/stimulus-bundle/assets/dist/loader.js',    ],```Those paths are now simple. One less thing to explain to people.Commits-------978b14d [AssetMapper] Allow simple, relative paths in importmap.php
This was referencedOct 21, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

[importmap] Wrong url for js modulepreload with site not at root of domain

4 participants

@weaverryan@stof@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp