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

Allow SlippiGame to accept an ArrayBuffer #77

Merged
merged 5 commits into from
Aug 16, 2021
Merged

Allow SlippiGame to accept an ArrayBuffer #77

merged 5 commits into from
Aug 16, 2021

Conversation

frankborden
Copy link
Contributor

@frankborden frankborden commented Jun 23, 2021

This makes it easier to use slippi-js in the browser via the Snowpack build tool, and probably for other build tools as well.

Snowpack can polyfill dependencies that need Node globals like Buffer by inlining them into the module of the dependency. That means slippi-js gets it's own Buffer library for internal use, but it's not possible for app code to create a Buffer object that will satisfy the instanceof check even if they use the same polyfill library. Passing in an ArrayBuffer will work on Node and in the browser.

@frankborden frankborden marked this pull request as ready for review June 23, 2021 22:48
@JLaferri
Copy link
Member

JLaferri commented Jul 6, 2021

With create-react-app slippi-js works fine out of the box using Node Buffers in the browser. Is there some reason your app can't do the same?

@frankborden
Copy link
Contributor Author

As mentioned in the discord, create-react-app does a polyfill to let you use Buffer in your app code because it's not node. With other web build tools this can be more difficult or not possible, even if they will polyfill the slippi-js library itself. I think this change expands support a good amount without compromising any of the internals or doing the extra work to rewrite all buffer usage to be natively compatible with browsers.

@vinceau vinceau merged commit af03a7b into project-slippi:master Aug 16, 2021
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