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

attachments: support attaching already-encoded base64 content #1552

Merged
merged 8 commits into from
Feb 2, 2021

Conversation

davidjgoss
Copy link
Contributor

Fixes #1550.

After the refactor to accomodate messages, we made it impossible to attach content that was already base64 encoded, since passing a string and mime type to the attach function will cause it to be set as "identity".

This makes it possible via a prefix on the mime type parameter e.g. base64:image/png. I'm not particularly wedded to this solution but it seems reasonable. Things we could do instead:

  • Add a third parameter to the attach function
  • Add an alternate function e.g. this.attachBase64
  • Auto-detect internally - possible but a lot of wasted computation as we'd need to base64 decode and then encode each string to understand if it started out base64 encoded or not

@coveralls
Copy link

coveralls commented Jan 25, 2021

Coverage Status

Coverage increased (+0.1%) to 98.076% when pulling 0ca6aa8 on attach-already-base64 into 1544bae on master.

Copy link

@hdorgeval hdorgeval left a comment

Choose a reason for hiding this comment

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

Hi @davidjgoss, If you allow me to suggest a comment:

mediaType in the context of the issue is mainly image/png and data is a string: so I expect that this string is already the result of a base64 encoding.

On line 54, I would do:

if ( typeof data === 'string' && mediaType.startsWith('image') {
  // assume this is already base64 encoded
  this.createStringAttachment(data, {
          encoding: messages.Attachment.ContentEncoding.BASE64,
          contentType: mediaType,
        })
}

In this case there is no need to add an extra prefix, and it is just a matter of clarifying the documentation that in such case the string is expected to be already base64 encoded by the caller of the attach method (as it was already the default behaviour in the v6 version)

@davidjgoss
Copy link
Contributor Author

@hdorgeval that's an option too, although even that has issues e.g. image/svg+xml.

CHANGELOG.md Outdated Show resolved Hide resolved
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.

HTML Formatter: Couldn't display image/png;base64 image because it wasn't base64 encoded
4 participants