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

ESBuild not possible when using flag-icons #1142

Open
joewIST opened this issue Aug 16, 2023 · 42 comments
Open

ESBuild not possible when using flag-icons #1142

joewIST opened this issue Aug 16, 2023 · 42 comments

Comments

@joewIST
Copy link

joewIST commented Aug 16, 2023

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
Copy link
Collaborator

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
Copy link
Author

joewIST commented Aug 21, 2023

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
Copy link
Author

joewIST commented Aug 31, 2023

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

@w0wka91
Copy link

w0wka91 commented Oct 20, 2023

Any solution for this issue?

@joewIST
Copy link
Author

joewIST commented Oct 20, 2023

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

@crodriguezdominguez
Copy link

Same problem here. I'm using version 6.14.0.

@ssougnez
Copy link

ssougnez commented Nov 9, 2023

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
Copy link

timothee-dhenain-zenika commented Nov 10, 2023

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
Copy link

w0wka91 commented Nov 11, 2023

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
Copy link

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

Thanks a lot for the tip, works like a charm

@joewIST
Copy link
Author

joewIST commented Nov 13, 2023

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

@ssougnez
Copy link

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

@joewIST
Copy link
Author

joewIST commented Nov 13, 2023

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

@ssougnez
Copy link

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
Copy link
Author

joewIST commented Nov 13, 2023

"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
Copy link

w0wka91 commented Nov 13, 2023

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
Copy link
Author

joewIST commented Nov 13, 2023

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
Copy link

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
Copy link
Author

joewIST commented Nov 13, 2023

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

@joewIST
Copy link
Author

joewIST commented Nov 16, 2023

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

@lipis
Copy link
Owner

lipis commented Nov 16, 2023

@joewIST What exactly should we do?

@ssougnez
Copy link

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

@lipis
Copy link
Owner

lipis commented Nov 16, 2023

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
Copy link

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
Copy link
Author

joewIST commented Nov 16, 2023

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
Copy link

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
Copy link
Collaborator

NotTsunami commented Nov 16, 2023

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
Copy link

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
Copy link
Author

joewIST commented Nov 17, 2023

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
Copy link
Owner

lipis commented Nov 17, 2023

@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
Copy link

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
Copy link
Collaborator

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
Copy link
Owner

lipis commented Nov 17, 2023

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

@tsteuwer-accesso
Copy link

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
Copy link
Collaborator

NotTsunami commented Dec 5, 2023

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:

  • Immediately add comment to README.md about usage of outputHashing as an immediate fix, but also provide a script that can be run as a build step to rename flags for cases where outputHashing is either not desired or insufficient.
  • In the very near future, choose a date to rename flags to xx-1x1.svg and xx-4x3.svg and then add a warning to the top of the README.md regarding when this change will occur.
  • In the longer term, enact on the name change.

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: [email protected]

@ssougnez
Copy link

ssougnez commented Dec 5, 2023

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
Copy link

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

@lipis
Copy link
Owner

lipis commented Dec 6, 2023

@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
Copy link

nkosi23 commented Jan 5, 2024

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

@NotTsunami NotTsunami pinned this issue Jan 5, 2024
@kristofdho
Copy link

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
Copy link

Ostabo commented Apr 11, 2024

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests