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

Fix corrupt images #55

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Fix corrupt images #55

merged 2 commits into from
Aug 27, 2024

Conversation

stevehanson
Copy link

@stevehanson stevehanson commented Aug 23, 2024

There is an issue where the assets images were all corrupted in the generated Belt app. This is occurring because Belt reads files as utf-8 text and then writes them when copying. It isn't handling binary files appropriately. The only binary files I’m aware of right now are in templates/boilerplate/assets.

To reproduce the original issue, from Belt project dir:

node bin/belt.js NewApp
cd builds/NewApp
open assets/splash.png
# corrupted image

This updates the copyTemplateDirectory function to use fs.copy for binary files (currently defined using an array of filenames that we identify as binary, but we might want something more robust in the future) instead of reading the file, optionally transforming it, and then writing it.

Copy link
Contributor

@codeofdiego codeofdiego left a comment

Choose a reason for hiding this comment

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

Looks great, just moved the list of unsupported extensions to the beginning of the file.

I also ran into an issue where I was still getting the images corrupted but that due to the files in the dist folder not being updated with the remote changes (ignored). But after running yarn build I was able to test it.

@codeofdiego codeofdiego merged commit 8ad7619 into main Aug 27, 2024
2 checks passed
@codeofdiego codeofdiego deleted the fix-corrupt-images branch August 27, 2024 15:59
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