10000 [AssetMapper] Add support for CSS files in the importmap by weaverryan · Pull Request #51543 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

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
Sep 29, 2023
Merged

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

merged 1 commit into from
Sep 29, 2023

Conversation

weaverryan
Copy link
Member
@weaverryan weaverryan commented Sep 4, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? fixes #51417 (fixed this along the way)
New feature? yes
Deprecations? no
Tickets None
License MIT
Doc PR TODO - 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

The importmap.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 JS import 'app.css'.

<script type="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 adds those to the importmap. This allows you to: import './styles/foo.css' without needing to add foo.css to the importmap. It "just works". This would result in foo.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 via importmap:require (or, if a package comes with a CSS file, it will be downloaded and added automatically - i.e. if the package has a style 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 from importmap.php that should be loaded - e.g. importmap('app') (the most common) or importmap(['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 if app.js imports other.js imports yet-another.js imports some_styles.css, using non-lazy imports (i.e. import './other.js vs the lazy import('./other.js')), then other.js, yet-another.js and some_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, if app.js is your entrypoint, then every 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 as link tags.

The preload option in importmap.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 as link tags). It finds all (in order) non-lazy imported CSS files from the entrypoint and render them as link tags in order (like Webpack).

I propose the recipe starting importmap.php is updated to be:

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

And then in assets/app.js:

import './styles/app.css';

C) Other Improvements

  • Fixed [importmap] Wrong url for js modulepreload with site not at root of domain #51417 where deploying to a subdirectory didn't work
  • Added a new event in asset-map:compile so other processes can hook into this (will make bundles like TailwindBundle) nicer.
  • Removed importmap:export command: I don't see a need for this
  • Added basic, conservative checking for commented-out imports - if lines START with // or /*
// both will correctly be ignored
// import 'foo';
/* import 'foo'; */

TODOs

  • Update the 6.4 docs
  • update the 6.4 recipe

@carsonbot carsonbot added this to the 6.4 milestone Sep 4, 2023
@weaverryan weaverryan force-pushed the asset-mapper-css-importmaps branch from 90f4bb3 to 7b3f810 Compare September 5, 2023 12:40
@weaverryan weaverryan force-pushed the asset-mapper-css-importmaps branch 2 times, most recently from fa22633 to 5ef0727 Compare September 13, 2023 12:35
nicolas-grekas
nicolas-grekas reviewed Sep 14, 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 played with this, it just works :)

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.

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
Member Author

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 that link tag will be removed. Instead, we'll either start the user's importmap.php with an app.css entry. Or (what I'm leaning to now) is exactly like Encore: in assets/app.js, you'd import './styles/app.css'. That would trigger the link tag.

@stof
Copy link
Member
stof commented Sep 14, 2023

Right now, I believe that link tag will be removed. Instead, we'll either start the user's importmap.php with an app.css entry. Or (what I'm leaning to now) is exactly like Encore: in assets/app.js, you'd import './styles/app.css'. That would trigger the link tag.

Please don't.
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.

@weaverryan
Copy link
Member Author

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' in importmap.php - super nice because you can importmap:require CSS files instead of needing to manage link tags manually or downloading vendor CSS if you want to avoid a CDN.

B) The ability to import './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 put app.css inside of importmap.php instead of importing it by default in app.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 main app.css file in importmap.php, then we could better encourage import CSS only when needed.

@stof
Copy link
Member
stof commented Sep 15, 2023

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.

@weaverryan
Copy link
Member Author

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 is preload => 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 is preload => 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 be preload => true either because it is directly in importmap.php with preload: 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 weaverryan force-pushed the asset-mapper-css-importmaps branch from c0618cf to ba79d06 Compare September 18, 2023 00:14
@weaverryan weaverryan mentioned this pull request Sep 18, 2023
Closed
@weaverryan
Copy link
Member Author

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 weaverryan mentioned this pull request Sep 21, 2023
Closed
@weaverryan weaverryan force-pushed the asset-mapper-css-importmaps branch from 84d94ae to 3e10302 Compare September 23, 2023 11:21
@weaverryan weaverryan mentioned this pull request Sep 23, 2023
Merged
@weaverryan
Copy link
Member Author

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 at https://github.com/weaverryan/ux/tree/assetmapper-css/ux.symfony.com

@weaverryan weaverryan mentioned this pull request Sep 24, 2023
Merged
@Chris53897 Chris53897 mentioned this pull request Sep 24, 2023
Open
@weaverryan weaverryan force-pushed the asset-mapper-css-importmaps branch 2 times, most recently from 724ba05 to 467dea6 Compare September 26, 2023 18:04
@weaverryan
Copy link
Member Author
weaverryan commented Sep 27, 2023 •
edited
Loading

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

@weaverryan weaverryan force-pushed the asset-mapper-css-importmaps branch from 467dea6 to 2020644 Compare September 28, 2023 00:48
@weaverryan
Copy link
Member Author

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

@weaverryan weaverryan force-pushed the asset-mapper-css-importmaps branch from a3d7e1c to e236cba Compare September 28, 2023 14:56
@weaverryan weaverryan force-pushed the asset-mapper-css-importmaps branch 2 times, most recently from 93a92eb to 9a00f0e Compare September 28, 2023 17:17
nicolas-grekas
nicolas-grekas reviewed Sep 29, 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.

LGTM, I just found minor things before approving 🙏

nicolas-grekas
nicolas-grekas reviewed Sep 29, 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.

please add a line to the changelog (of the twig-bridge also I guess)

src/Symfony/Bridge/Twig/Extension/ImportMapRuntime.php Outdated
@@ -22,7 +22,7 @@ public function __construct(private readonly ImportMapRenderer $importMapRendere
{
}

public function importmap(?string $entryPoint = 'app', array $attributes = []): string
public function importmap(string|array $entryPoint, array $attributes = []): string
Copy link
Member

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 use the 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 implement app 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 passing null then if we have a deprecation layer.

Copy link
Member Author

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 left app as the default (in part for BC). But in the recipe, I WOULD like to start people with importmap('app') because it's more explicit.

@nicolas-grekas nicolas-grekas force-pushed the asset-mapper-css-importmaps branch from 391e272 to 5060fa1 Compare September 29, 2023 17:12
nicolas-grekas
nicolas-grekas approved these changes Sep 29, 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.

(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.

@nicolas-grekas nicolas-grekas merged commit 4f6f404 into symfony:6.4 Sep 29, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Sep 29, 2023
Closed
@weaverryan weaverryan deleted the asset-mapper-css-importmaps branch September 29, 2023 17:18
nicolas-grekas added a commit that referenced this pull request Sep 29, 2023
@nicolas-grekas
feature #51729 [AssetMapper] Allow simple, relative paths in importma…
0b78897 
…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        | TODO

This 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 referenced Oct 21, 2023
Merged
Merged
@MatTheCat MatTheCat mentioned this pull request Apr 8, 2024
Open
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

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

Assignees
No one assigned
Labels
AssetMapper Feature Status: Reviewed
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
0