Generate a default `favicon.ico` in `public` folder, don't generate one on compile #2625

FranjoMindek · 2025-04-04T14:16:53Z

New projects start with favicon.ico inside of public directory.
If there is no favicon.ico we still exclude the favicon <link> tag from the <head> of index.html.

Fixes #1979

Question for reviewers: Do we, and where do we notify users of new behavior in docs?

TODO: golden tests

FranjoMindek · 2025-04-07T07:47:10Z

waspc/src/Wasp/Generator/WebAppGenerator.hs

+    faviconFd = C.mkTmplFd . C.asTmplFile $ [relfile|public/favicon.ico|]
+    extPublicFileDrafts = map C.mkPublicFileDraft $ AS.externalPublicFiles spec
    templateData =
      object
        [ "title" .= (AS.App.title (snd $ getApp spec) :: String),
-          "head" .= (maybe "" (intercalate "\n") (AS.App.head $ snd $ getApp spec) :: String)
+          "head"
+            .= (maybe "" (intercalate "\n") (AS.App.head $ snd $ getApp spec) :: String),
+          "isFaviconPresent" .= checkIfFileDraftExists faviconFd extPublicFileDrafts


Something feels off here

Again, old approach, but let's improve it.

This code wants to know something about user-defined file paths. Instead of directly checking what it wants to know, it first transofrms file paths into file drafts, and then does the (slightly incorrect) check there.

We should extract the knowledge we need as close to the source data as possible, not later down the road after further procesing. In fact, this function doesn't even need the favicon file draft. It's creating one just to check something about the data needed to construct it.

Think about what to do here and expand when you figure it out:

Here's how I'd do it, compare it with your approach:

"isFaviconPresent" .= didUserProvideFavicon -- ... didUserProvideFavicon = didUserProvidePublicFile faviconPathInPublicDir $ AS.externalPublicFiles spec faviconPathInPublicDir = [relfile|public/favicon.ico|] -- This used to be `checkIfFileDraftExists` didUserProvidePublicFile :: ...

Copilot

Copilot reviewed 4 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (3)

waspc/data/Generator/templates/react-app/index.html: Language not supported
waspc/src/Wasp/Generator/WebAppGenerator.hs: Language not supported
waspc/waspc.cabal: Language not supported

FranjoMindek · 2025-04-07T12:47:52Z

Copilot reviewed 4 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (3)

waspc/data/Generator/templates/react-app/index.html: Language not supported

waspc/src/Wasp/Generator/WebAppGenerator.hs: Language not supported

waspc/waspc.cabal: Language not supported

Bro can't do Haskell

sodic · 2025-04-08T09:20:01Z

Bro can't do Haskell

Not over yet... Btw, is this ready for review? I see it's still a draft.

FranjoMindek · 2025-04-08T09:27:42Z

It's ready for review

infomiho · 2025-04-09T15:31:25Z

Question for reviewers: Do we, and where do we notify users of new behavior in docs?

If it's a breaking change, we put it in the migration docs 👍 e.g. https://wasp.sh/docs/migration-guides/migrate-from-0-15-to-0-16

waspc/ChangeLog.md

waspc/data/Generator/templates/react-app/index.html

web/docs/tutorial/02-project-structure.md

infomiho · 2025-04-09T15:40:47Z

@FranjoMindek did I get the logic correctly:

include the default favicon.ico
users can delete that and our code no longer generated the <link> tag in the HTML file
if users decide to use e.g. favicon.svg they need to include their own <link>?

I'll review this PR more after you confirm I understood 👍

# Conflicts: # waspc/ChangeLog.md

FranjoMindek · 2025-04-10T09:30:38Z

@FranjoMindek did I get the logic correctly:

include the default favicon.ico

users can delete that and our code no longer generated the <link> tag in the HTML file

if users decide to use e.g. favicon.svg they need to include their own <link>?

I'll review this PR more after you confirm I understood 👍

Yes.
I'll create the discussion we talked about in #wasp-dev discord channel.

sodic

Nice work noticing something's off with the code!

I left some comments that explain it.

web/docs/tutorial/02-project-structure.md

web/docs/migration-guides/migrate-from-0-16-to-0-17.md

sodic · 2025-04-10T10:35:12Z

waspc/src/Wasp/Generator/WebAppGenerator.hs

+checkIfFileDraftExists :: FileDraft -> [FileDraft] -> Bool
+checkIfFileDraftExists draftToCheck drafts = dstPathToCheck `elem` dstPaths
+  where
+    dstPaths = map FD.getDstPath drafts
+    dstPathToCheck = FD.getDstPath draftToCheck


This function claims to check whether a file draft exists (and implies to be comparing file drafts), but it only compares their paths. We should either:

Change its signature to Path -> [Path] -> Bool, which just turns it into a wrapper around elem.

Improve its name.

Remove it.

Also, it seems to be comparing destination paths instead of source paths, which coincidentally works but is semantically incorrect. Thefore, I vote for removing replacing the function with a more appropriate one (more on this in another comment).

This is also old code I extracted out since it was used on two places (and possibly more in future).
Didn't want to do any bigger changes with File System since that part of Haskell and Wasp is still a bit unclear to me. And whole logic on how it works was a bit confusing e.g. why are destination paths compared to compare file drafts. I'll try to understand it a bit more first then and see how to improve it.

sodic · 2025-04-10T11:12:03Z

waspc/src/Wasp/Generator/WebAppGenerator.hs

      ++ ifUserDidntProvideFile genManifestFd
  where
    publicFiles = AS.externalPublicFiles spec
    extPublicFileDrafts = map C.mkPublicFileDraft publicFiles
-    genFaviconFd = C.mkTmplFd (C.asTmplFile [relfile|public/favicon.ico|])
    genManifestFd = C.mkTmplFdWithData tmplFile tmplData


Old code, but this function's name is wrong - gen implies it's a generator monad but it's just a file draft.

sodic · 2025-04-10T11:40:03Z

waspc/src/Wasp/Generator/WebAppGenerator.hs

+    faviconFd = C.mkTmplFd . C.asTmplFile $ [relfile|public/favicon.ico|]
+    extPublicFileDrafts = map C.mkPublicFileDraft $ AS.externalPublicFiles spec
    templateData =
      object
        [ "title" .= (AS.App.title (snd $ getApp spec) :: String),
-          "head" .= (maybe "" (intercalate "\n") (AS.App.head $ snd $ getApp spec) :: String)
+          "head"
+            .= (maybe "" (intercalate "\n") (AS.App.head $ snd $ getApp spec) :: String),
+          "isFaviconPresent" .= checkIfFileDraftExists faviconFd extPublicFileDrafts


Again, old approach, but let's improve it.

This code wants to know something about user-defined file paths. Instead of directly checking what it wants to know, it first transofrms file paths into file drafts, and then does the (slightly incorrect) check there.

We should extract the knowledge we need as close to the source data as possible, not later down the road after further procesing. In fact, this function doesn't even need the favicon file draft. It's creating one just to check something about the data needed to construct it.

Think about what to do here and expand when you figure it out:

Here's how I'd do it, compare it with your approach:

"isFaviconPresent" .= didUserProvideFavicon -- ... didUserProvideFavicon = didUserProvidePublicFile faviconPathInPublicDir $ AS.externalPublicFiles spec faviconPathInPublicDir = [relfile|public/favicon.ico|] -- This used to be `checkIfFileDraftExists` didUserProvidePublicFile :: ...

FranjoMindek · 2025-04-11T08:44:16Z

According to pool on discord, we will refactor the current solution so that the link tags are provided in the wasp configs head property.

infomiho

Left some comments, mostly about the prose

waspc/ChangeLog.md

infomiho · 2025-04-15T12:16:18Z

waspc/ChangeLog.md


 ### 🔧 Small improvements

- Show a friendlier error when there are no routes defined in the wasp file ([#2643](https://github.com/wasp-lang/wasp/pull/2643))
+- Show a friendlier error when there are no routes defined in the wasp file ([#2643](https://github.com/wasp-lang/wasp/pull/2643)).


Auto format?

No, I've seen older versions had them so I manually re-added them for all missing ones

infomiho · 2025-04-15T12:16:39Z

waspc/data/Cli/templates/basic/main.wasp

 }

 route RootRoute { path: "/", to: MainPage }
 page MainPage {
  component: import { MainPage } from "@src/MainPage"
 }
+


Missing newline

infomiho · 2025-04-15T12:17:56Z

waspc/src/Wasp/Generator/WebAppGenerator.hs

@@ -161,13 +161,11 @@ genPublicDir :: AppSpec -> Generator [FileDraft]
 genPublicDir spec =
  return $
    extPublicFileDrafts
-      ++ ifUserDidntProvideFile genFaviconFd
-      ++ ifUserDidntProvideFile genManifestFd
+      ++ ifUserDidntProvideFile manifestFd


Does it make sense to do the same thing for manifest.json as we did for favicon.ico? 🤷

With time possibly yes, that would be an another issue though.

I'm more for user doing configuration in userland, but I know Matija and Martin probably want to also keep it magical. The question is where to draw the line.

infomiho · 2025-04-15T12:24:42Z

web/docs/migration-guides/migrate-from-0-16-to-0-17.md

+### 2. Add a default `favicon.ico` to the public folder
+
+This step is necessary only if you have no `favicon.ico` in your `public` folder.
+In that case add a default `favicon.ico` to your `public` folder.


Suggested change

In that case add a default `favicon.ico` to your `public` folder.

You should add a `favicon.ico` to your `public` folder. If you want to keep the default, download it here: [link].

I'd add the link to download the default for people that don't really care / don't have a special favicon in mind. This is a great place to link to a favicon generator for people that want to have their logo as favicon.

Do we have any go-to recommendation for favicon generators?

Not really, this is a soft recommendation, what ever feels okay for you as a dev, people with less experience will appreciate your tip.

Added a link to https://realfavicongenerator.net/

web/docs/migration-guides/migrate-from-0-16-to-0-17.md

Co-authored-by: Mihovil Ilakovac <[email protected]>

# Conflicts: # waspc/ChangeLog.md # web/docs/migration-guides/migrate-from-0-16-to-0-17.md # web/docs/tutorial/02-project-structure.md

infomiho

Let's make sure the CI is passing.

I really like this change, it makes things easier to understand without reading any docs on how to customize stuff, you see the file, you change it and it just works.

FranjoMindek added 8 commits March 24, 2025 14:27

Savepoint

07a6339

Update docs to feature default favicon.ico

b64fccf

Merge remote-tracking branch 'origin/main' into franjo/fb/favicon

11b1404

Add changelog

0430bed

Don't generate favicon, check for favicon in index

47c5642

Add breaking change

ec049ab

Revert rel change from pasting

64a1570

Remove shortcut because its outdated

23fcf4c

FranjoMindek commented Apr 7, 2025

View reviewed changes

FranjoMindek requested a review from Copilot April 7, 2025 12:46

Copilot AI reviewed Apr 7, 2025

View reviewed changes

fix changelog

01e2085

FranjoMindek marked this pull request as ready for review April 8, 2025 09:27

infomiho reviewed Apr 9, 2025

View reviewed changes

waspc/ChangeLog.md Outdated Show resolved Hide resolved

infomiho reviewed Apr 9, 2025

View reviewed changes

waspc/data/Generator/templates/react-app/index.html Outdated Show resolved Hide resolved

infomiho reviewed Apr 9, 2025

View reviewed changes

web/docs/tutorial/02-project-structure.md Outdated Show resolved Hide resolved

FranjoMindek added 4 commits April 10, 2025 11:08

ver -> unreleased

8402a4a

Merge remote-tracking branch 'origin/main' into franjo/fb/favicon

31f2155

# Conflicts: # waspc/ChangeLog.md

whitespaces

46baaa1

add migration guide

709ee94

cprecioso mentioned this pull request Apr 10, 2025

Add migration guide for #2598 #2647

Merged

4 tasks

sodic reviewed Apr 10, 2025

View reviewed changes

revert favicon changes add head to wasp template

0eaaa04

infomiho reviewed Apr 15, 2025

View reviewed changes

FranjoMindek and others added 16 commits April 15, 2025 17:12

Update waspc/ChangeLog.md

cbcd3c4

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update waspc/ChangeLog.md

4ac5bf2

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

28eecd4

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

d2986ca

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

348add1

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

b0f6804

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

e271238

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

561f27f

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

b5eb52b

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

212f47d

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

0b89598

Co-authored-by: Mihovil Ilakovac <[email protected]>

Update web/docs/migration-guides/migrate-from-0-16-to-0-17.md

5b03080

Co-authored-by: Mihovil Ilakovac <[email protected]>

remove carlos stuff

1e16797

newline

2c09653

link

53de1e6

wording

cb0b8ea

infomiho mentioned this pull request Apr 16, 2025

Have a proper favicon support #2628

Closed

FranjoMindek requested a review from infomiho April 17, 2025 09:06

FranjoMindek self-assigned this Apr 18, 2025

Merge remote-tracking branch 'origin/main' into franjo/fb/favicon

c4c4b74

# Conflicts: # waspc/ChangeLog.md # web/docs/migration-guides/migrate-from-0-16-to-0-17.md # web/docs/tutorial/02-project-structure.md

infomiho approved these changes Apr 28, 2025

View reviewed changes

FranjoMindek added 4 commits April 28, 2025 14:53

Merge branch 'main' into franjo/fb/favicon

1532dc7

golden tests

e563f6e

add favicon tool link

23d4caa

remove favicon from filedraft test

40e184a

FranjoMindek merged commit 75db3d3 into main Apr 28, 2025
6 checks passed

FranjoMindek deleted the franjo/fb/favicon branch April 28, 2025 13:40

This was referenced Jul 14, 2025

Make Wasp AI work with binary files #2951

Open

Automatically test Wasp AI #2953

Open

	In that case add a default `favicon.ico` to your `public` folder.
	You should add a `favicon.ico` to your `public` folder. If you want to keep the default, download it here: [link].

Uh oh!

Generate a default favicon.ico in public folder, don't generate one on compile #2625

Generate a default favicon.ico in public folder, don't generate one on compile #2625

Uh oh!

Conversation

FranjoMindek commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

FranjoMindek commented Apr 7, 2025

Uh oh!

sodic commented Apr 8, 2025

Uh oh!

FranjoMindek commented Apr 8, 2025

Uh oh!

infomiho commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

infomiho commented Apr 9, 2025

Uh oh!

FranjoMindek commented Apr 10, 2025

Uh oh!

sodic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FranjoMindek commented Apr 11, 2025

Uh oh!

infomiho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

infomiho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Generate a default `favicon.ico` in `public` folder, don't generate one on compile #2625

Generate a default `favicon.ico` in `public` folder, don't generate one on compile #2625

FranjoMindek commented Apr 4, 2025 •

edited

Loading

infomiho commented Apr 9, 2025 •

edited

Loading