Promises example added to README.md#62
Conversation
jescalan
left a comment
There was a problem hiding this comment.
So I really appreciate the pull request here, and I hope it was a good process going through it! The work you did here is great, and the pull request is well-formatted and clear.
Unfortunately, I'm not sure this is a relevant addition to the documentation. The example given, while wrapped in a promise, doesn't actually need to use a promise at all, and the same functionality can be implemented with a simple function, as seen in the comments. So while it definitely is a cool exercise in javascript to run this transformation, it's more of a lesson in general javascript data transformation than an addition to the documentation that is specific to spike records.
| } | ||
| }); | ||
| }; | ||
| ``` |
There was a problem hiding this comment.
I'm not sure I understand why this needs to be in a separate file...
There was a problem hiding this comment.
Also, there is no async action in here, so it doesn't need to be wrapped in a promise at all. What you are really trying to do here is just a synchronous transformation on your data, which could just be encapsulated by a normal function. So in app.js:
const reduce = require('object.reduce')
const menuObj = { pepperoniPizza: 10.50, cheesePizza: 8.50, cinaStix: 5.00 }
module.exports = {
// ...
plugins: [new Records({
addDataTo: locals,
four: { data: transformPizza(menuObj) },
})]
// ...
}
function transformPizza (obj) {
const pizzas = reduce(obj, (m, v, k) => {
if (k.includes('Pizza')) m.push({ type: v.replace('Pizza', ''), price: `$${v}` })
return m
}, [])
if (!pizzas.length) throw new Error("There's no pizza :(")
return pizzas
}There was a problem hiding this comment.
Very true, nothing Asynchronous going on. I wanted to keep things simple, but that doesn't really make any sense to not have it be asynchronous. I'll make some changes and add another commit.
There was a problem hiding this comment.
Sounds good! Generally, I think just saying "you can pass a function that returns a promise and it will work" is pretty sufficient for the docs for this library, without going into deep detail on what promises are or how to use them, since that's kind of out of scope for this specific library.
However, I think something like this would make a fantastic blog post that a lot of people could learn from, and which I would be happy to link out to from the docs, if you're into that!
| price: "$".concat(${obj[prop]}) | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be done more efficiently as such:
const reduce = require('object.reduce')
const pizzas = reduce(obj, (m, v, k) => {
if (k.includes('Pizza')) m.push({ type: v.replace('Pizza', ''), price: `$${v}` })
return m
}, [])| resolve(pizzas); | ||
| } else { | ||
| reject(Error("There's no pizza :(")); | ||
| } |
There was a problem hiding this comment.
This is a good candidate for one line:
pizzas.length ? resolve(pizzas) : reject("There's no pizza :(")
It took me a little bit to figure out the syntax for using promises, so I figured I would add something about it to README.md since it's a bit sparse. I tried to keep it all consistent with existing formatting, and I included a brief description as well as an example.
The description is a bit longer than
fileorurl, but I think with the increased complexity it's justified. That being said, The example could be a bit more in-depth than is necessary, but I don't normally write documentation so I have no idea.More than happy to move the promise out of the separate module file, let me know what you think.