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

handle plugin buffer response #950

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

FredKSchott
Copy link
Owner

@FredKSchott FredKSchott commented Aug 28, 2020

Changes

Testing

  • Manually tested (We don't have good testing for third party plugins at the moment)

@FredKSchott FredKSchott requested a review from a team as a code owner August 28, 2020 01:55
@vercel
Copy link

vercel bot commented Aug 28, 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/pnp6srqnl
✅ Preview: https://snowpack-git-handle-buffer-plugin-response-type.pikapkg.vercel.app

@gr2m
Copy link
Contributor

gr2m commented Aug 28, 2020

I've followed @jaredLunde's "Option 1" instructions here: #945 (reply in thread)

I've run yarn dev and then the local snowpack from this pull request side-by-side, but I don't see any differences. I was expecting to see a different image size? What am I looking for?

@jaredLunde
Copy link
Contributor

@gr2m I updated the plugin to return {result: Buffer} instead of just a binary string so neither option will be broken anymore. Previously the image file was corrupted.

@gr2m
Copy link
Contributor

gr2m commented Aug 28, 2020

I have reverted this change directly in node_modules/snowpack-plugin-resize-images/dist/main/index.js. I saw the image being corrupted. But it was still corrupted when I run snowpack dev from this pull request.

Did you check if this pull request fixes the problem for you?

@jaredLunde
Copy link
Contributor

jaredLunde commented Aug 28, 2020

The old plugin returned a binary string. It will still be corrupted w/ this PR because this doesn't attempt to handle binary strings, it just fixes a bug where you couldn't return just a buffer. You had to return an object {result: Buffer}. @stramel covered the issues around handling binary strings in the discussion.

Sequence of events was something like:

  1. I try returning a Buffer from my sharp plugin, that doesn't work
  2. I convert it a binary string instead, which works in 2.9.0
  3. 2.9.1 converts everything to a Buffer, but accidentally breaks support for binary strings in the process
  4. I find in the source that I can return a Buffer if I put it in an object under the result key
  5. fks submits this PR making it possible to return a Buffer without putting it in an object

@gr2m
Copy link
Contributor

gr2m commented Aug 28, 2020

We don't have good testing for third party plugins at the moment, hmm

@FredKSchott could we create a testcase with a minimal plugin local to the test?

@FredKSchott
Copy link
Owner Author

FredKSchott commented Aug 29, 2020

@gr2m I'd rather not implement a new test suite as a part of this PR, but I definitely agree that we're overdue. I actually think that testing would be a great place for you to dig into to set up these things for the long term.

Added a note that this has been manually tested

@FredKSchott FredKSchott merged commit b112148 into master Aug 31, 2020
@FredKSchott FredKSchott deleted the handle-buffer-plugin-response-type branch August 31, 2020 23:39
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

4 participants