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

Improve encoding #882

Merged
merged 1 commit into from
Aug 25, 2020
Merged

Improve encoding #882

merged 1 commit into from
Aug 25, 2020

Conversation

drwpow
Copy link
Collaborator

@drwpow drwpow commented Aug 23, 2020

Changes

Snowpack used 'binary' as the encoding for binary files, but apparently that’s not what it means! It’s actually an alias for latin1 meant for text files. For binary files, there’s no encoding, so nothing should be returned instead. This PR allows Snowpack to work with Buffers (binary files) more directly both in build and dev.

Credit to @stramel for the find/suggestion!

Hopefully this will fix reported weirdness with binary files, such as https://www.pika.dev/npm/snowpack/discuss/505 and problems with video files @stramel noted.

Testing

Binary files work just fine in the build & dev server.

@drwpow drwpow requested a review from a team as a code owner August 23, 2020 04:09
@vercel
Copy link

vercel bot commented Aug 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/e8ld3l1m7
✅ Preview: https://snowpack-git-drwpow-fix-encoding.pikapkg.vercel.app

@drwpow drwpow changed the title Fix encoding Improve encoding Aug 23, 2020
export function getEncodingType(ext: string): 'utf-8' | 'binary' {
return UTF8_FORMATS.includes(ext) ? 'utf-8' : 'binary';
export function getEncodingType(ext: string): 'utf-8' | undefined {
return UTF8_FORMATS.includes(ext) ? 'utf-8' : undefined;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the core change here; everything else is basically type changes from this

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

TIL!

Copy link
Contributor

@stramel stramel 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! This should make things easier for plugins handling binary files.

@@ -184,23 +184,25 @@ export async function wrapImportProxy({
config,
}: {
url: string;
code: string;
code: string | Buffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about replacing all references to "code" with "content"? This should be a bit more consistent since we're no longer only handling code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I agree, and tried to replace it, and realized that’s part of our plugin API now—a load() plugin can provide a { code: string, map: string } object if it’s generating source maps. Even though that’s still an “experimental” API, we should still double-check if we want to make that API change. Spun up an issue here: #910

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.

None yet

3 participants