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.

Sign up for GitHub

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

Jump to bottom

feat: improve icon generator, use icon*.NODE_ENV.png #172

Merged
merged 5 commits into from
Sep 3, 2022
Merged

feat: improve icon generator, use icon*.NODE_ENV.png #172

merged 5 commits into from
Sep 3, 2022

Conversation

gitpash
Copy link
Contributor

@gitpash gitpash commented Sep 3, 2022 •
edited

Details

Code of Conduct

  • I agree to follow this project's Code of Conduct
  • I checked the current PR for duplication.

louisgv
louisgv reviewed Sep 3, 2022
View reviewed changes
Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

Interesting fallback strategy. This can be further improved to work for other assets as well!

@gitpash
Copy link
Contributor Author

gitpash commented Sep 3, 2022

Interesting fallback strategy. This can be further improved to work for other assets as well!

Thanks! I'm also thinking this is a bit narrow and too explicit, for general assets management can be implemented some sort of pattern matching, but i'm not sure what exactly as of now..

@louisgv louisgv mentioned this pull request Sep 3, 2022
Closed
@louisgv
Copy link
Contributor

louisgv commented Sep 3, 2022

@gitpash can you review and test out the new image generator? I did some fun caching as well as applying the environment variable into the name for both base icons, as well as developer-provided icons as well!

@louisgv louisgv changed the title feat: use icon*.development.png feat: improve icon generator, use icon*.NODE_ENV.png, and more Sep 3, 2022
@louisgv louisgv changed the title feat: improve icon generator, use icon*.NODE_ENV.png, and more feat: improve icon generator, use icon*.NODE_ENV.png Sep 3, 2022
ColdSauce
ColdSauce reviewed Sep 3, 2022
View reviewed changes
cli/plasmo/src/features/helpers/crypto.ts
/**
* Fast hash for local file revving
* DO NOT USE FOR SENSITIVE PURPOSES
* md5 is good enough for file-revving: https://github.com/sindresorhus/rev-hash
Copy link
Contributor

Choose a reason for hiding this comment

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

we can even do crc32 for file diff if we want to squeeze out perf

Copy link
Contributor

@louisgv louisgv Sep 3, 2022 •
edited

Choose a reason for hiding this comment

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

I think even google was struggling at implementing usable crc32 for their storage lol, they had to defer it to another module las time I checked xd

cli/plasmo/src/features/extension-devtools/project-watcher.ts Outdated Show resolved Hide resolved
cli/plasmo/src/features/extension-devtools/generate-icons.ts
await Promise.all(
[128, 48, 32, 16].map((width) => {
// Cache the dev provided icon paths for each width
if (iconState.devProvidedIcons[width] === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we invalidate this cache?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just static path cache to avoid extra path resolution. We would invalidate it against the passed down assetDirectory, but that path doesn't change at runtime atm xD

@louisgv @ColdSauce
Update cli/plasmo/src/features/extension-devtools/project-watcher.ts
3c311f8 
Co-authored-by: Stefan Aleksic <soccerfanatic1996@gmail.com>
Signed-off-by: L <6723574+louisgv@users.noreply.github.com>
@louisgv louisgv merged commit 148e94c into PlasmoHQ:main Sep 3, 2022
@gitpash
Copy link
Contributor Author

gitpash commented Sep 5, 2022

@gitpash can you review and test out the new image generator? I did some fun caching as well as applying the environment variable into the name for both base icons, as well as developer-provided icons as well!

It doesn't work for me, although it may be due to my local config, basically what I'm doing is running the fork of Plasmo and test in one of the examples directory.. when I log iconNameList I'm having

iconNameList:  [
  'icon.undefined.png',
  'icon.png',
  'icon512.undefined.png',
  'icon512.png',
  'icon-512.undefined.png',
  'icon-512.png',
  'icon-512x512.undefined.png',
  'icon-512x512.png',
  'icon1024.undefined.png',
  'icon1024.png',
  'icon-1024.undefined.png',
  'icon-1024.png',
  'icon-1024x1024.undefined.png',
  'icon-1024x1024.png'
]

@louisgv
Copy link
Contributor

louisgv commented Sep 6, 2022

oh oops... we set the environment variable at run-time, so the iconNameList must be created at run-time before the variable is set. By putting it at top-level, it turned into undefined! Derp, I'mma push a fix out soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@louisgv louisgv louisgv left review comments

@ColdSauce ColdSauce ColdSauce approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC] icon*.development.png
4 participants
@gitpash @louisgv @ColdSauce @pavelanymind