lipis / flag-icons

:flags: A curated collection of all country flags in SVG — plus the CSS for easier integration
https://flagicons.lipis.dev
MIT License
10.79k stars 1.86k forks source link

ESBuild not possible when using flag-icons #1142

Open joewIST opened 1 year ago

joewIST commented 1 year ago

We have an Angular application inside an Nx monorepo. I'd like to start using ESBuild as a builder for improved build speeds, but we are currently blocked by flag-icons. The following errors occur and the build fails:

Two output files share the same path but have different contents: xx.svg

Is there anything we can do to avoid/fix this?

Much appreciated!

NotTsunami commented 1 year ago

Would you be able to provide a minimal reproducible repository? I suspect esbuild is trying to place 1x1 and 4x3 resolutions in the same output folder.

joewIST commented 1 year ago

I'm afraid I can't provide a repository, but I can refer to some of the features of the build:

We are using: "flag-icons": "6.9.5",

The Angular build executor is using: "executor": "@angular-devkit/build-angular:browser-esbuild",

The ts.config.base.json is using: "target": "ESNext", "module": "ESNext"

And you are correct: ESBuild seems to be putting both resolutions in the same folder, but they have identical names.

I found a workaround which was to put:

script in the index.html, but this is sub-optimal IMO as it means we would need to update the package in both package.json and index.html.

Any help would be greatly appreciated!

joewIST commented 1 year ago

Hi, is this something that is fixable or should we look towards alternative options?

w0wka91 commented 1 year ago

Any solution for this issue?

joewIST commented 1 year ago

I have still not found a solution and therefore we cannot use esbuild in our project yet.

crodriguezdominguez commented 1 year ago

Same problem here. I'm using version 6.14.0.

ssougnez commented 1 year ago

As previous posts indicated, the issue is this:

.fi-gw {
  background-image: url(../flags/4x3/gw.svg);
}
.fi-gw.fis {
  background-image: url(../flags/1x1/gw.svg);
}

Both file will be copied in the same directory with the same name. A solution would be to modify the CSS of flag-icons to:

.fi-gw {
  background-image: url(../flags/4x3/gw-4x3.svg);
}
.fi-gw.fis {
  background-image: url(../flags/1x1/gw-1x1.svg);
}

This way, image names won't collide anymore.

timothee-dhenain-zenika commented 1 year ago

Having the same issue here, we tried to upgrade our project with the Angular latest release (v17) , but the application bundle generation fails with the new ESBuild

I don't have a minimal reproducible repository, but on our side we have this configuration:

package.json

"architect": {
        "build": {
          "builder": "@angular-devkit/build-angular:application",
          "options": {
            "styles": ["src/styles.scss", "node_modules/flag-icons/sass/flag-icons.scss"],

Including these styles in the build process means that they will be compiled and bundled with the rest of the application's assets when we build the Angular project. If you need, some further documentation is provided here

As a upgrade-blocking issue, if you could fix it that'd be great!

w0wka91 commented 1 year ago

You can fix the issue by adding outputHashing to the development configuration:

          "configurations": {
            "production": {
              "budgets": [
                {
                  "type": "initial",
                  "maximumWarning": "500kb",
                  "maximumError": "1mb"
                },
                {
                  "type": "anyComponentStyle",
                  "maximumWarning": "2kb",
                  "maximumError": "4kb"
                }
              ],
              "outputHashing": "all"
            },
            "development": {
              "optimization": false,
              "extractLicenses": false,
              "sourceMap": true,
              "outputHashing": "media"
            }
          }
ssougnez commented 1 year ago

Well, know I know what is the purpose of this outputHashing stuff.

Thanks a lot for the tip, works like a charm

joewIST commented 1 year ago

This hasn't worked for us, we still get the same errors. This is now blocking us from updating to Angular v.17.

ssougnez commented 1 year ago

Ensure that you apply it to the correct configuration. I was stuck with migrating to ng17 and this worked for me.

joewIST commented 1 year ago

I have copied the above and still get the same issues...

ssougnez commented 1 year ago

can you maybe post your whole angular.json in here (not doubting you, just trying to find out the difference with mine to help you)?

joewIST commented 1 year ago

"options": { "baseHref": "/", "outputPath": "dist/apps/XXXX/browser/browser", "index": "apps/XXXX/src/index.html", "main": "apps/XXXX/src/main.ts", "tsConfig": "apps/XXXX/tsconfig.app.json", "polyfills": "apps/XXXX/src/polyfills.ts", "assets": [ "apps/XXXX/src/assets/icon", "apps/XXXX/src/favicon.ico", "apps/XXXX/src/manifest.webmanifest", { "input": "apps/XXXX/src/assets/config/development", "glob": "*.json", "output": "assets/config" }, { "input": "libs/shared/ui/assets", "glob": "/", "output": "assets" } ], "styles": [ "node_modules/@angular/material/prebuilt-themes/deeppurple-amber.css", "node_modules/katex/dist/katex.css", "node_modules/flag-icons/css/flag-icons.min.css", "node_modules/intro.js/introjs.css", "apps/XXXX/src/styles.scss" ], "stylePreprocessorOptions": { "includePaths": ["apps/XXXX/src/styles"] }, "scripts": ["node_modules/webfontloader/webfontloader.js", "node_modules/intro.js/intro.js"], "allowedCommonJsDependencies": ["dayjs", "katex", "extract-math"], "inlineStyleLanguage": "scss", "customWebpackConfig": { "path": "./webpack.config.js" } }, "configurations": { "defaultConfiguration": "development", "development": { "buildOptimizer": false, "optimization": false, "vendorChunk": true, "extractLicenses": false, "sourceMap": true, "namedChunks": true }, "production": { "serviceWorker": true, "ngswConfigPath": "apps/XXXX/ngsw-config.json", "assets": [ "apps/XXXX/src/assets/icon", "apps/XXXX/src/favicon.ico", "apps/XXXX/src/manifest.webmanifest", { "input": "apps/XXXX/src/assets/config/production", "glob": ".json", "output": "assets/config" }, { "input": "libs/shared/ui/assets", "glob": "/*", "output": "assets" } ], "sourceMap": { "scripts": true }, "optimization": true, "outputHashing": "all", "namedChunks": false, "extractLicenses": true, "vendorChunk": false, "buildOptimizer": true, "fileReplacements": [ { "replace": "apps/XXXX/src/environments/environment.ts", "with": "apps/XXXX/src/environments/environment.prod.ts" } ] } }

w0wka91 commented 1 year ago

You need to add outputHashing to your development configuration:

"options": {
"baseHref": "/",
"outputPath": "dist/apps/XXXX/browser/browser",
"index": "apps/XXXX/src/index.html",
"main": "apps/XXXX/src/main.ts",
"tsConfig": "apps/XXXX/tsconfig.app.json",
"polyfills": "apps/XXXX/src/polyfills.ts",
"assets": [
"apps/XXXX/src/assets/icon",
"apps/XXXX/src/favicon.ico",
"apps/XXXX/src/manifest.webmanifest",
{
"input": "apps/XXXX/src/assets/config/development",
"glob": ".json",
"output": "assets/config"
},
{
"input": "libs/shared/ui/assets",
"glob": "**/",
"output": "assets"
}
],
"styles": [
"node_modules/@angular/material/prebuilt-themes/deeppurple-amber.css",
"node_modules/katex/dist/katex.css",
"node_modules/flag-icons/css/flag-icons.min.css",
"node_modules/intro.js/introjs.css",
"apps/XXXX/src/styles.scss"
],
"stylePreprocessorOptions": {
"includePaths": ["apps/XXXX/src/styles"]
},
"scripts": ["node_modules/webfontloader/webfontloader.js", "node_modules/intro.js/intro.js"],
"allowedCommonJsDependencies": ["dayjs", "katex", "extract-math"],
"inlineStyleLanguage": "scss",
"customWebpackConfig": {
"path": "./webpack.config.js"
}
},
"configurations": {
"defaultConfiguration": "development",
"development": {
"outputHashing": "media",
"buildOptimizer": false,
"optimization": false,
"vendorChunk": true,
"extractLicenses": false,
"sourceMap": true,
"namedChunks": true
},
"production": {
"serviceWorker": true,
"ngswConfigPath": "apps/XXXX/ngsw-config.json",
"assets": [
"apps/XXXX/src/assets/icon",
"apps/XXXX/src/favicon.ico",
"apps/XXXX/src/manifest.webmanifest",
{
"input": "apps/XXXX/src/assets/config/production",
"glob": ".json",
"output": "assets/config"
},
{
"input": "libs/shared/ui/assets",
"glob": "**/",
"output": "assets"
}
],
"sourceMap": {
"scripts": true
},
"optimization": true,
"outputHashing": "all",
"namedChunks": false,
"extractLicenses": true,
"vendorChunk": false,
"buildOptimizer": true,
"fileReplacements": [
{
"replace": "apps/XXXX/src/environments/environment.ts",
"with": "apps/XXXX/src/environments/environment.prod.ts"
}
]
}
}
joewIST commented 1 year ago

Ah sorry I'd just removed that while reverting my changes. It still doesn't work so I will need to continue using webpack browser

ssougnez commented 1 year ago

When I changed the build to application, I had to change this line:

"main": "apps/XXXX/src/main.ts",

to

"browser": "apps/XXXX/src/main.ts",

but I see that it's not the case for you. Can you try ? And ensure that you're using the "application" builder?

joewIST commented 1 year ago

Yes I already tried that as well, no luck...

joewIST commented 1 year ago

Is this something this library is planning on supporting? @lipis

lipis commented 1 year ago

@joewIST What exactly should we do?

ssougnez commented 1 year ago

Renaming the files as I suggested might be a viable solution.

lipis commented 1 year ago

Renaming the files? Haven't really followed closely the conversation.. all we offer in this library is bunch of files and the css for it.. there is no .ts or anything like that...

ssougnez commented 1 year ago

The issue is that when using esbuilder, all files end up in the same directory and as files in 1x1 have the same names as the ones in 4x3, it creates an override issue.

There is a possibility to configure esbuilder to hash the names but it seems that it does not work for everyone.

Therefore, a solution would be to rename files in 1x1 with the suffix "-1x1" and same for "4x3".

joewIST commented 1 year ago

Yes the duplicate file issue is basically blocking us from upgrading from esbuild, and therefore from fully taking advantage of Angular v.17. I have tried outputHashing but it's not working...

btxtiger commented 1 year ago

While enabling outputHashing works for our application, it still has to be considered as workaround since it can impact the build performance in dev env, especially for big enterprise apps.

More about esbuild asset hashing: https://esbuild.github.io/api/#asset-names

Following this, there is no reason that the duplicate file issue of this library remains with the hashing enabled. You should recheck the esbuild configuration in your project. https://angular.io/guide/esbuild https://nx.dev/nx-api/esbuild

That being said, the main issue is that esbuild in general conflicts with the architecture of this library. Its not an Angular issue.

NotTsunami commented 1 year ago

The issue is that when using esbuilder, all files end up in the same directory and as files in 1x1 have the same names as the ones in 4x3, it creates an override issue.

There is a possibility to configure esbuilder to hash the names but it seems that it does not work for everyone.

Therefore, a solution would be to rename files in 1x1 with the suffix "-1x1" and same for "4x3".

Keep in mind the renaming for the files will be a breaking change as well. If you'd like, I can provide a batch script in the project root that appends the -1x1 and -4x3 to the respective flags (and maybe one to perhaps undo it?), and then you should be able to invoke that from your package.json via npm explore in your build steps. Does that sound like an appropriate response in the case where outputHashing isn't the right solution? I'm not opposed to other solutions as well, but I'm a little hesitant to just rename flags when that's a breaking change.

ssougnez commented 1 year ago

To be honest, the outputHash works with me, so it does not matter.

However, people are not supposed to use images directly, are they? So if you adapt the scss to the new image names, why would it be a breaking change?

joewIST commented 1 year ago

OK I managed to get outputHashing to work, but I needed to put it in the "options" part of the build configuration, not in the "configurations" -> "development" section. Not sure why but that works. We are still blocked from fully integrating esbuild due to our reliance on sentry (which needs a webpack plugin; no support for esbuild plugins yet without custom implementation), but at least I know I can do a workaround for this library. Thanks @ssougnez !

lipis commented 1 year ago

@ssougnez not many people are using the css at all but directly the images (including me in some projects).. the css is an add on.. the main product is the flag icons as the name suggests :)

ssougnez commented 1 year ago

Yeah sorry, I'm still on the old one which was flags-icon-css or something wasn't it? Ok then, I understand the breaking change :-)

Now, it seems that the one who had an issue with outputHash fixed it so maybe the issue is no longer relevant.

NotTsunami commented 1 year ago

At the very least, I'll push a change to the README.md referencing this issue for ESBuild usage. If there's any desire for a script to rename the flags on the fly, I can add that on-demand.

lipis commented 1 year ago

I'm down on updating the Readme and adding a script for those users if needed...

tsteuwer-accesso commented 1 year ago

I personally dont like hashing images if they're not changing. Because that would mean that browsers will need to redownload them everytime a new build is released. Is it possible for us to just rename the file or add the files so that this isn't an issue with bundlers?

NotTsunami commented 12 months ago

I personally dont like hashing images if they're not changing. Because that would mean that browsers will need to redownload them everytime a new build is released. Is it possible for us to just rename the file or add the files so that this isn't an issue with bundlers?

As previously mentioned, the reason I'm hesitant to push a change like that is that there are several packages/libraries/applications that link directly to the flags inside the repo (which isn't really encouraged, but it is fairly common), so this can break resolution if we suddenly change with no advance notice.

I guess ultimately, we should aim to:

That would be the path of least resistance. Naturally, there will still be people that won't read this and we would run into issues, but that's unavoidable. I've noticed from working on JavaScript projects that sometimes when you pull packages they include notices or comments, like depcrecation warnings for renamed packages, etc. I wonder if we can make use of that to provide additional notice regarding renaming the flags if we end up choosing to do so. Thoughts? @lipis

Also updated my public facing email, feel free to bring this over to email as well: contact@tylerd.dev

ssougnez commented 12 months ago

To be honest, that's what major versions are for... No one should upgrade a library to a new major version without paying attention to the breaking changes.

If you include a clear explanation in the readme about it, it should be enough.

tsteuwer-accesso commented 12 months ago

I agree with @ssougnez . The change should just be a move to new file names and make it a new major version.

lipis commented 12 months ago

@NotTsunami Let's agree on the final naming and we can do it eventually.. yes no need to announce anything.. new major version with breaking changes.. who ever is using the website for showing the flags I don't care to be honest.. it's not fair anyway :)

nkosi23 commented 11 months ago

i've just been beaten up by this after switching to esbuild

kristofdho commented 8 months ago

Still can't get it to work with "builder": "@angular-devkit/build-angular:browser-esbuild", and outputHashing set in various different places. Any idea when a next major release for this would be planned?

Ostabo commented 7 months ago

I made a fork as a temporary solution for people like me who can't wait:
repo: Ostabo/flag-icons - npm: @ostabo/flag-icons
File names are changed like this: flags/1x1/ad.svg -> flags/1x1-ad.svg

Beware tho that this is no drop in replacement as imports need to change

olafurfannsker commented 7 months ago

The outputHashing worked like a charm with flag-icons v: 7.2.1 and angular v: 17.3.5...

itworksafisher commented 1 week ago

Production builds should really use output hashing none though. Output hashing is all about cache busting. Setting it to media is going to add a hash to all the flag icons, i.e. ad.svg turns in to something like ad.09812309a8sd09.svg, which is how this setting fixes the issue with two files with same name. They won't be the same anymore because they will have different hashes. But this will apply to ALL media, not just the flag icons. The flag icons are small, so this isn't a concern, but for any media which is large, you will be forcing the browser to redownload it with each new build, even when it hasn't changed. This doesn't seem like a good solution overall.

joewIST commented 1 week ago

Is this something that @lipis would consider updating any time soon?

itworksafisher commented 1 week ago

A good example from my project is that we host font awesome pro fonts, which can be upward of 1.4MB for the ttfs (woff2 is much smaller). We definitely would want that to be cached if possible, but using outputHashing media prevents that.