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

options.logo support string array #332

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Conversation

carlhopf
Copy link
Contributor

favicons >= 7.0.0 picks source image by it's size if provided with an array

solves #265

@carlhopf carlhopf changed the title support string array for logo argument options.logo support string array Oct 11, 2022
Copy link
Collaborator

@andy128k andy128k left a comment

Choose a reason for hiding this comment

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

Great work!

My only concern is that only first file's hash is used for caching. Cache invalidation is known to be a hard thing ;).

May you check if this change #334 can be useful here?

src/index.js Outdated
// Recompile filesystem cache if any source based path change:
(fileSources) =>
getRelativeOutputPath(
fileSources[0].hash,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to hash all files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Smth like getContentHash(...fileSources.map(s => s.content)) should work.

src/index.js Outdated
const resolvedPublicPath = getResolvedPublicPath(
logo.hash,
logoFileSources[0].hash,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hash all files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

amended 👍

favicons >= 7.0.0 picks source image by it's size,
solves jantimon#265
@carlhopf carlhopf force-pushed the logo-string-array branch 3 times, most recently from 9f781aa to c3aeb7d Compare October 12, 2022 18:40
@carlhopf
Copy link
Contributor Author

thanks the review 👍 let me know if the changes work, and i'll go ahead and update/rebase the maskable pr

@andy128k
Copy link
Collaborator

@carlhopf Oops. It seems, you've changed too much. Now getRelativeOutputPath is invoked later and this.options are not considered in cache calculations.

I pulled your previous commit and made a change on top of it. Please have a look here #335 . As far as I can see, all tests are passed.

@carlhopf
Copy link
Contributor Author

ah took me a bit to wrap my head around the internals ;) thanks for the example 👍 updated commit pushed

@andy128k andy128k merged commit 805b726 into jantimon:main Oct 12, 2022
@andy128k
Copy link
Collaborator

Thanks!

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