Skip to content

Updating images and fonts to use Webpack 5 Asset modules #883

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

Merged
merged 7 commits into from
Jan 22, 2021

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Jan 9, 2021

This is one more big piece for proper Webpack 5 support - see #880

This also updates the CHANGELOG both for this PR and for the original in #645

And, this adds a new assertion method that helps check directory contents in functional.js in a way that is more resilient to hash changes.

For the most part... this just worked! See the CHANGELOG. Because this will release on 1.0, I have decided to remove a few methods instead of deprecating them ).

Cheers!

@weaverryan weaverryan mentioned this pull request Jan 9, 2021
10 tasks
Comment on lines +199 to +201
* The expectedFiles can contain a [hash:8] syntax in case
* the file is versioned - e.g. main.[hash:8].js, which would
* match a real file like main.abcd1234.js.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

this.useFontsLoader = true;
this.imageRuleOptions = {
type: 'asset',
maxSize: null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't override the default value for maxSize.

Based on the documentation everything under 8kb will be inlined... which isn't necessarily a bad thing but it will probably confuse some people/break things (for instance if someone used to import images in order to trigger a copy).

Copy link
Member Author

Choose a reason for hiding this comment

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

which isn't necessarily a bad thing but it will probably confuse some people/break things

I think this will also cause some confusion. On the other hand, if you have:

import image from './images.foo.png';

img.src = image;

I think this will work regardless of whether it was inlined or copied. Unless we defaulted this to 0 (or used asset/resource instead of assets by default), this will surprise people.

So, I'm conflicted: on the one hand, the inlining of 8kb and lower is probably better for performance... but will cause more confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to go the "more conservative" route and use asset/resource by default

This will help avoid confusion because images won't be inlined by default.
@weaverryan weaverryan merged commit 7d9248f into symfony:main Jan 22, 2021
@weaverryan weaverryan deleted the asset-modules branch January 22, 2021 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants