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

Removed base-64 package in favour of node Buffer #68

Merged
merged 2 commits into from
Nov 7, 2018

Conversation

8eecf0d2
Copy link
Contributor

@8eecf0d2 8eecf0d2 commented Oct 17, 2018

I haven't read much about the base-64 package but it's usage in netlify-lambda must be incorrect or not doing what's expected.

Switched to node Buffer instead and afaict it should be doing the same thing, but for whatever reason Buffer works and base-64 doesn't 🤷‍♂️

#26

@swyxio
Copy link
Contributor

swyxio commented Oct 18, 2018

thanks Brod. will circulate for review here

@swyxio swyxio requested review from DavidWells and bcomnes October 18, 2018 00:05
Copy link
Contributor

@bcomnes bcomnes left a comment

Choose a reason for hiding this comment

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

lgtm, but I don't have the bandwidth to test this at the moment. @sw-yx do you have time to test?

@8eecf0d2
Copy link
Contributor Author

8eecf0d2 commented Oct 19, 2018

I'm also a bit lost regarding this feature..

As far as responses, the following should return a png to the browser - changing isBase64Encoded to false should return the base64 string.

import * as fs from "fs";

export const handler = async (request, context, callback) => {

  const pngBuffer = fs.readFileSync("image.png");
  
  return callback(null, {
    statusCode: 200,
    body: pngBuffer.toString("base64"),
    isBase64Encoded: true,
  })
}

As far as request's go, Netlify Functions docs mentions:

{
  ...
  "isBase64Encoded": "A boolean flag to indicate if the applicable request payload is Base64-encode"
}

Which I'm guessing from the previous implementation worked correctly(?) although I have no means to test it right now.

@bcomnes
Copy link
Contributor

bcomnes commented Oct 19, 2018

cc @biilmann any insight on whats going on here?

@swyxio
Copy link
Contributor

swyxio commented Nov 7, 2018

accepting as it seems to be the better way but not sure if it's actually fixing #26 .

@swyxio swyxio merged commit aa89890 into netlify:master Nov 7, 2018
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.

3 participants