search

aheckmann / node-ses

An Amazon SES api for nodejs with proper error handling.
http://aheckmann.github.com/node-ses
MIT License
201 stars 37 forks source link

Support for json type results. #45

Closed jamonkko closed 4 years ago

jamonkko commented 7 years ago

AWS API and request library have both good support for json format, so it would be possible just use those without any xml parsing needed.

Left the xml as default type so that the change is fully backwards compatible. All functionality is exactly the same as before unless if you specify the resultType: 'json' option which will then just use the request library's json: true mode and consequently ask AWS to return result as json format. request will also automatically parse the json to object before callbacking.

Since JavaScript users might prefer JSON over XML, maybe it could be considered also later on to switch jsonresultType to be default when releasing version 3 and backwards compatibility can be broken. Would still keep the XML option also.

markstos commented 7 years ago

Good idea. I'll review.

markstos commented 7 years ago

Most users don't care about data format is used on the wire, since they use this module to extract away the API calls. From reading the docs, we don't really expose XML to users anywhere except in the data field returned by errors, which I expect is rarely used.

Are you aware of other non-backwards-compatible changes that users would experience if we made our API calls in JSON?

Since most users don't care about the format, I'm not interested in exposing an option which would be hardly used.

I do think there is some benefit to switching to JSON to remove a dependency and to improve performance silently.

Assuming there's little to know breaking change exposed, I would rather make the change not optional and bump the version number appropriate if there's a breaking change.

jamonkko commented 7 years ago

That sounds like much better plan and would clean up code also a lot. I think the only backwards compatibility that would break would be that users would get object data value in callback instead of data that is string of XML, which would be fine to break if bumping up the major version.

I can make the changes and do new commit If you think this is the way to go ?

markstos commented 7 years ago

Sounds good, please go ahead.

On Tue, Jun 6, 2017 at 3:38 PM Jarkko Mönkkönen notifications@github.com wrote:

That sounds like much better plan and would clean up code also a lot. I think the only backwards compatibility that would break would be that users would get object data value in callback instead of data that is string of XML, which would be fine to break if bumping up the major version.

I can make the changes and do new commit If you think this is the way to go ?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/aheckmann/node-ses/pull/45#issuecomment-306594514, or mute the thread https://github.com/notifications/unsubscribe-auth/AABk5bkJ7hs_m_sz_y2DyXr2Xf4PyxK7ks5sBaqZgaJpZM4NwkNs .

jamonkko commented 7 years ago

New version with only json format (object). Removed the xml dependency also.

markstos commented 7 years ago

Thanks, I've made a note to review this.

markstos commented 7 years ago

I've reviewed this more now and it looks good.

While we are making a breaking change, this seems like a good time to also consider supporting a promise-based API, which I saw you also have a branch for.

If we used Bluebird's asCallback(), we could fairly easily support both callback and promise-based APIs.

One difference with the Promise-based API would be that promises should only have one return value, but our callbacks currently have two. One solution would be for the promises to callback with [data, res].

See: https://github.com/haoliangyu/node-ses-any-promise/commit/d089210a8382f0eb1ffca7c18b264e8050f41200#commitcomment-22519422 for related discussion.

Would you be interested in contributing a promise-based API using Bluebird's asCallback()?

I'm not interested in trying to support any-promise, since they don't all support the asCallback() method I think we should use. If someone wants to use a different promise library, they can call Promise.resolve() with the library of their choice on the promise that we would return.

Ref: http://bluebirdjs.com/docs/api/ascallback.html

(Promise-supposed can become a new issue/pull-request if you are interested to pursue it).

jamonkko commented 7 years ago

I think it would be great to have the promise support. I would prefer it to be separate pull request, to not bundle all that stuff to the same change, but still of course makes sense to release them together.

That asCallback solution looks very good. By the way, what about dropping returning the res value, or is there use case for it? Would data be enough?

I'm not completely sure if I have time this week for implementing promise support and therefore am little hesitant to committing for it.

markstos commented 4 years ago

This was merged and release today in node-ses@3.0.0. Thanks!