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

feat: Accept Uint8Array as file code alongside string #773

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

izznat
Copy link

@izznat izznat commented Feb 28, 2023

What kind of change does this pull request introduce?

Feature implementation for #769 .

What is the current behavior?

#769

Sandpack only acceptsfile code in string format even though nodebox accept both string and Uin8Array.

What is the new behavior?

Sandpack accepts both string and Uin8Array as file code.

What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.

Not yet tested.

Checklist

  • Documentation;
  • Storybook (if applicable);
  • Tests;
  • Ready to be merged;

- adjust SandpackBundlerFile and SandpackFile types to accept Uin8Array
- create `codeToString` utils to convert Uint8Array into utf-8 string using `TextDecoder`
@vercel
Copy link

vercel bot commented Feb 28, 2023

@izznatsir is attempting to deploy a commit to the CodeSandbox Team on Vercel.

A member of the Team first needs to authorize it.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9f70efc:

Sandbox Source
Sandpack Configuration
sandpack 2 example not updating with customSetup and files in the provider Configuration

Comment on lines +20 to +27
export function codeToString(code: string | Uint8Array): string {
if (typeof code === "string") {
return code;
} else {
const decoder = new TextDecoder();
return decoder.decode(code);
}
}
Copy link
Author

Choose a reason for hiding this comment

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

There is also readBuffer function in node runtime utils that do the same thing but using Buffer.from(Uint8Array).toString('utf-8') instead of TextDecoder. Should I remove this and update readBuffer implementation to use TextDecoder so it can be used outside nodebox?

@izznat
Copy link
Author

izznat commented Feb 28, 2023

SandpackFile and SandpackBundlerFile has no information about what encoding is suitable to encode or decode the code. So, all are treated as utf-8. Not all files are encodable into utf-8, but TextDecoder does not understand binary string encoding that is available in Node environment. So, maybe we should add another field in SandpackFile and SanpackBundlerFile called encoding that accepts either binary or utf-8, default to utf-8.

The encoding field can be used to determine whether a file is viewable, so useActiveCode util will return undefined if the encoding is binary and code viewer can display a message that the file is not viewable. This is not implemented yet, what do you think?

Or, we can create a list of file extensions that are encodable into utf-8 and check that in useActiveCode function.

@DeMoorJasper
Copy link
Member

Not sure if encoding is really neccessary here, the only runtime that can handle binary data right now is nodebox and there we just write the binary data to the filesystem directly, the only time we need encoding is for reading the pkg.json in sandpack. We can also use something like this if we wanna detect binary for image previews or something: https://github.com/alessioalex/is-binary

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