Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrading to use AssetMapper 6.4 #1449

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Nov 7, 2023

Hi!

This converts from Encore to AssetMapper 6.4 (and also upgrading to Symfony 6.4 in general). This was the final product of a live stream walking through this process - https://www.youtube.com/watch?v=pyj1hvhcxGc

There are still some TODOs to finish. The biggest is the first, as Sass is quite embedded in there.

  • A) convert Sass->CSS (not a requirement for AssetMapper, but Javier & I talked about this). Then remove SassBundle & bootstrap in composer.json. Probably many Sass variables should be converted to CSS variables. See Remove Sass from custom CSS files #1450 and Import the entire Bootstrap styles #1452. Much of this was done, Sass kept just for Bootstrap.
  • B) In assets/app.js, I should just be able to import from the individual bootstrap package instead of requiring each part It's fine how it is. When we upgrade, perhaps we could look at it then.
  • C) commit public/assets/ or assets/vendor/ so the code works after clone with ZERO other commands. Else, the user will need to run importmap:install.
  • D) update devcontainer.json
  • E) fix the admin entrypoint & layout

It's a busy month! If anyone is interested in trying to fix any of the above points, just let me know here or on Slack. I'd be more than happy to coach someone through them.

Cheers!

javiereguiluz added a commit that referenced this pull request Nov 9, 2023
This PR was squashed before being merged into the main branch.

Discussion
----------

Remove Sass from custom CSS files

This is mentioned by `@weaverryan` in #1449.

We don't really use any Sass feature (except nesting and some variables). I've replaced Sass variables by CSS custom properties ... and removed the nesting by flattening all styles (we can readd nesting soon when native support is a big bigger than today: https://caniuse.com/?search=nesting).

**Ryan**, if you agree on this, can we just merge it and then you rebase your PR? Thanks!

Commits
-------

691859e Remove Sass from custom CSS files
javiereguiluz added a commit that referenced this pull request Nov 10, 2023
This PR was merged into the main branch.

Discussion
----------

Import the entire Bootstrap styles

Here's another proposal to improve `@weaverryan`'s work on #1449.

We already import 90% of Bootstrap so, why not import 100% of it?

Commits
-------

bae0115 Import the entire Bootstrap styles
templates/base.html.twig Outdated Show resolved Hide resolved
@tacman
Copy link
Contributor

tacman commented Nov 19, 2023

commit public/assets/ or assets/vendor/ so the code works after clone with ZERO other commands. Else, the user will need to run importmap:install.

I think committing those is going to create a worse DX experience. Developers download the demo and start tweaking it as a way of learning Symfony's best practices. I've been burned with accidentally having the public/assets and wondering why stimulus or css wasn't working.

bash git clone && composer install && assets:install && start the server

That's a pretty amazing installation. And it makes sense -- download the PHP code, download the front-end assets, start the server. We shouldn't commit the front-end assets (js/css/images) for the same reason we don't commit the /vendor directory.

@tacman
Copy link
Contributor

tacman commented Nov 19, 2023

I'm happy to help, maybe with the admin layout?

@javiereguiluz
Copy link
Member

@tacman I'm sorry but we don't plant to change the current way of working and installing with this demo.

One of its best features is that it doesn't require anything except cloning and composer install. No extra config. No extra asset installing or generation. Nothing.

This feature has made the Symfony Demo to be usable in many different scenarios. For example, the PHP source code uses Symfony Demo in heir continuous benchmarks. That's only possible because of how easy is to install and use it.

Thanks for understanding!

@tacman
Copy link
Contributor

tacman commented Nov 19, 2023 via email

@RubenKruiswijk
Copy link

@tacman Wouldn't that be as simple as adding the assetmapper equivalent to https://github.com/weaverryan/symfony-demo/blob/upgrading-to-asset-mapper/composer.json#L95?

@tacman
Copy link
Contributor

tacman commented Nov 29, 2023

@RubenKruiswijk Indeed, my applications all have

        "auto-scripts": {
            "cache:clear": "symfony-cmd",
            "assets:install %PUBLIC_DIR%": "symfony-cmd",
            "fos:js-routing:dump --format=js --target=public/js/fos_js_routes.js --callback=\"export default  \"": "symfony-cmd"
        },

Any for deployment, I have the same in either Procfile or app.json (to deploy to heroku / dokku)

web:  vendor/bin/heroku-php-nginx -C nginx.conf  -F fpm_custom.conf public/
release: bin/console importmap:install && bin/console asset-map:compile
    "scripts": {
        "dokku": {
            "predeploy": "bin\/console importmap:install && bin\/console asset-map:compile",
            "postdeploy": ""
        }
    },

I still think putting the public/assets in the repo is going to be a pain point for many developers.

@stof
Copy link
Member

stof commented Nov 29, 2023

For heroku, I would recommend to do that in the heroku-postdeploy composer script (which is run during deployment) instead of doing that in the release step (which will run in a dedicated dyno and so won't impact the web dynos if your files are served by your app and not a CDN).

@stof
Copy link
Member

stof commented Nov 29, 2023

And the importmap:install should even be in auto-scripts so that any composer install runs it automatically (maybe it could be added in the recipe)

ogizanagi added a commit to StenopePHP/skeleton that referenced this pull request Dec 8, 2023
To experiment with asset mapper & stenope, according to
StenopePHP/Stenope#173 issue.

Also, this will be the future of assets handling with Symfony, no more
bundler implied :
symfony/demo#1449 (comment)
weaverryan added a commit to symfony/webpack-encore that referenced this pull request Dec 9, 2023
This PR was merged into the main branch.

Discussion
----------

docs: document asset mapper and remove demo link

Related to symfony/demo#1449 (review)

Commits
-------

a798bf7 docs: document asset mapper and remove demo link
@javiereguiluz
Copy link
Member

Ryan, thanks for keep working on this 🙌

I recently upgraded a full Symfony app from Webpack Encore to AssetMapper. I have some comments and questions about AssetMapper and shared them at symfony/symfony#52939

When this PR is finished, I'll look in detail at Ryan's work to add or remove more comments/questions. Thanks!

@weaverryan
Copy link
Member Author

This is now ready! Thanks Javier for the other PR's to smooth my path :).

In some future PR's, I think it's time to modernize a few things also (I'm sure you're already aware) - like going to Bootstrap 5, removing jQuery, etc. Some of the complexity of the PR is due to some older things being used, but I'm happy with the result! I don't know the demo super well, so I did my best to check things.

@javiereguiluz
Copy link
Member

Just asking: to keep the previous behavior where assets are fully compiled and ready to use out-of-the-box, shouldn't we commit the assets/vendor/ dir to the repo too?

@weaverryan
Copy link
Member Author

Just asking: to keep the previous behavior where assets are fully compiled and ready to use out-of-the-box, shouldn't we commit the assets/vendor/ dir to the repo too?

Very fair question. I added importmap:install to the composer scripts. So if you run composer install, everything is there. So if you somehow run composer install which you must do somehow even right now, then you should have everything you need. But if you experience something different, please tell me!

@javiereguiluz
Copy link
Member

Ryan, thanks a lot for working so hard on this contribution 🙇

Let's merge it now so folks can play with it before tagging a new release. Thanks!

@javiereguiluz javiereguiluz merged commit b0c6921 into symfony:main Dec 11, 2023
6 of 7 checks passed
@weaverryan weaverryan deleted the upgrading-to-asset-mapper branch December 11, 2023 14:17
javiereguiluz added a commit that referenced this pull request Dec 11, 2023
This PR was merged into the main branch.

Discussion
----------

Add a missing command to build Sass files

After merging #1449 I saw this error when running the app:

![image](https://github.com/symfony/demo/assets/73419/5969dde6-0ecf-4ed4-bd37-14084371c2e4)

Commits
-------

4137bb5 Add a missing command to build Sass files
@weaverryan weaverryan changed the title [WIP] Upgrading to AssetMapper 6.4 Upgrading to AssetMapper 6.4 Dec 11, 2023
@weaverryan weaverryan changed the title Upgrading to AssetMapper 6.4 Upgrading to use AssetMapper 6.4 Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants