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(adapter-fetch): Add support for handling binary data #332

Merged
merged 4 commits into from
May 16, 2020

Conversation

offirgolan
Copy link
Collaborator

@offirgolan offirgolan commented May 14, 2020

Description

Add support for handling binary data.

Motivation and Context

#183

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My code follows the code style of this project.
  • My commits and the title of this PR follow the Conventional Commits Specification.
  • I have read the contributing guidelines.

"detect-node": "^2.0.4"
"buffer": "^5.6.0",
"detect-node": "^2.0.4",
"to-arraybuffer": "^1.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

to-arraybuffer uses require('buffer').Buffer so I'm confused how this all is built in our current build pipeline - is it some how "browserifying" the node implementation and also bundling the npm "buffer" package?

Copy link
Collaborator Author

@offirgolan offirgolan May 15, 2020

Choose a reason for hiding this comment

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

Ok so looked more into it to see what was actually being built. We current have rollup-plugin-node-builtins setup which has support for using Buffer via buffer-es6.

If I imported the buffer via import { Buffer } from 'buffer' it only inlines the polyfill for the UMD build but when I used from 'buffer/', it inlined it for all 3 builds (CJS, ES, and UMD). Since this is only meant for the browser, it would make sense for it to be be polyfilled in all 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explains a lot, was very confused - thanks for looking into it

@@ -3,8 +3,6 @@ node_js:
- '10'
- '12'

dist: trusty
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 dist is EOL and is no longer supported by Chrome. That was the reason for the failing builds.

@offirgolan offirgolan requested a review from jasonmit May 15, 2020 19:54
@offirgolan offirgolan merged commit 111bebf into master May 16, 2020
@offirgolan offirgolan deleted the adapter-fetch-support-binary-data branch May 16, 2020 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants