Add convert method #17
Conversation
| setXHRHeaders(request, options.headers) | ||
| request.send() | ||
| }) | ||
| } |
There was a problem hiding this comment.
Not sure if there is an explicit browser-support contract, but any reason not to use fetch here? Seems like any browser that supports Object#assign would also support fetch.
There was a problem hiding this comment.
I'd like to be backward-compatible at least down to IE11 (I shipped polyfilled versions today in v1.2.0 as I realized there were some holes). I picked core-js for polyfills (the same as Babel uses), and unfortunately, it doesn't provide one for Fetch.
I want to avoid adding dependencies for each feature, so let's wait for this one and refactor later when it's time to drop IE altogether.
src/services/helpers.js
Outdated
| encodeURIComponent(object[key]) | ||
| ].join('') | ||
| }, '') | ||
| } |
There was a problem hiding this comment.
Similar question: URLSearchParams? No Edge 16 support, but pretty good support otherwise. Might be able to use just URL which does have better support.
There was a problem hiding this comment.
URL isn't polyfilled in core-js yet, let's use it when it is.
|
@scotttrinh (and/or anyone else) Changed a few things and made the method more API-proof. Would love a final review before merge if possible 🙂 Overview of the meaningful changes:
|
src/services/settings.js
Outdated
| @@ -28,16 +28,33 @@ export const Defaults = { | |||
| * @property {String} globalFormat - The global format for new Dinero objects (see {@link module:Dinero~toFormat toFormat} for format). | |||
| * @property {String} globalRoundingMode - The global rounding mode for new Dinero objects (see {@link module:Dinero~multiply multiply} or {@link module:Dinero~divide divide} for format). | |||
| * @property {String} globalFormatRoundingMode - The global rounding mode to format new Dinero objects (see {@link module:Dinero~toFormat toFormat} or {@link module:Dinero~toRoundedUnit toRoundedUnit} for format). | |||
| * @property {String} globalExchangeRatesApi.endpoint - The global exchange rates API endpoint for new Dinero objects (see {@link module:Dinero~convert convert} for format). | |||
| * @property {String} globalExchangeRatesApi.JSONPath - The global exchange rates API JSON path for new Dinero objects (see {@link module:Dinero~convert convert} for format). | |||
There was a problem hiding this comment.
Do you think it might be a bit misleading to call this JSONPath since it doesn't seem to actually be JSONPath?
Lodash has a similar functionality which takes arrays into account which we might consider using here so that there is a well-understood/tested interface.
There was a problem hiding this comment.
I did some testing and you can access arrays by using the index 'b.c.1', so that works for me, but I'm still not sure we should call it JSONPath just because of the ambiguity.
There was a problem hiding this comment.
You're totally right about the name ambiguity. I'll go for propertyPath.
|
@scotttrinh Cool! Merging it now, deploying it probably later next week. Thanks for the review! |
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR adds a
convertmethod toDinero, allowing for users to convert Dinero objects from one currency to another.Users need to plug their own forex API to retrieve exchange rates. The
convertmethod queries aForexservice which calls the API and returns a promise with the right exchange rate for the destination currency. Then,convertcreates a new Dinero object with the converted amount and new currency, and returns it within a promise.