Closed SpecialAro closed 1 month ago
@SpecialAro - after building using both the main app and the recipes (your branches), I still don't see the icon.svg
to be deleted in the installation profile folder.
@SpecialAro - after building using both the main app and the recipes (your branches), I still don't see the
icon.svg
to be deleted in the installation profile folder.
Huum... That is weird. Can you check what you get in this debug?
debug('Removing default icon', defaultIcon);
if (defaultIcon) {
removeSync(join(recipeDirectory, 'icon.svg'));
}
for pre-existing recipes in a pre-existing profile, the icon.svg
does not get auto-deleted UNLESS there's a version bump in the recipe. For eg, I tried for the gmail service (installed recipe), and the icon.svg
remained until I bumped up the version of gmail recipe and repackaged. Then, Ferdium replaces the whole folder and prompts to "Reload services"
Update: This is not such a big deal. In fact, if I forcibly remove the icon.svg
file without a version bump, then some other (db?) reference is still present to the non-existent file on the local file-system, which then makes the sidebar show up without any icons. So, probably this auto-delete not working is a boon in disguise!
Oh! Maybe I wasn't clear enough, sorry. I meant to say that SVGs are only updated after a version bump (that is why I said to locally bump down the version on your profile - to simulate an update).
I purposely added code (and didn't remove it) to we ensure that all users still get a functional Ferdium with icons, without a breaking change. This way, after merging, we can trigger a version bump on all recipes (we can make a PR just for bumping all recipes) and, consequently, users will see their SVGs gone, but will have the new versions with defaultIcons
defined.
Does that make sense? Do you agree with this?
Note: one thing that I didn't test/accounted for is the custom recipes. But I think that, for those, we can keep the possibility of having a local SVG (so we would remove the TODO comment and leave the code as permanent).
yes, i agree. if needed, we can do the version-bump for all recipes.
Thank you very much @vraravam!
Should we also test on Linux before merging (and also remove the debug statement and the TODO comment?)
(I'm heading off, but I can do that tomorrow) 😁
Should we also test on Linux before merging (and also remove the debug statement and the TODO comment?)
i dont think there's any OS-specific code in there. So, it should work in linux too.
I can remove the debug statements and the TODO comments now
Pre-flight Checklist
Please ensure you've completed all of the following.
Description of Change
Reduce user bundle on recipes by removing SVG local reference
Motivation and Context
After some discussion with @vraravam we came to the conclusion that SVG increase local bundle size for users, which is unnecessary given users can fetch the static assets generated by
jsdelivr
.My idea is that we can still have some control over the icons by making use
jsdelivr
cdn to point to GH assets for theicon.svg
of each recipe, whilst giving the possibility for users to create (or modify existing recipes) by making use ofdefaultIcon
key (that should contain an URL or be undefined - more in the twin PR here: )To test this PR do the following:
recipes
submodule and add myferdium-recipes
fork as a remotepnpm i
andpnpm package
%AppData%/Ferdium/recipes
) and manually bring the version down of each recipepackage.json
What to expect:
%AppData%/Ferdium/recipes/{recipeId}
folder should be automatically deleted.This PR needs to be merged AFTER the one on the recipes repo (https://github.com/ferdium/ferdium-recipes/pull/537).
This PR was tested in:
Checklist
pnpm prepare-code
)pnpm test
passes