Added transformRaw option to modify source data before JSON parsing #73
Conversation
| @@ -107,7 +107,7 @@ function renderFile (root, obj) { | |||
| } | |||
|
|
|||
| function renderUrl (obj) { | |||
| return rest(obj.url).then((res) => { return JSON.parse(res.entity) }) | |||
| return rest(obj.url).then((res) => { return JSON.parse(transformRaw(obj, res.entity)) }) | |||
There was a problem hiding this comment.
Ok so in this case the transformRaw option would only work for url sources. I think this is ok, but should be noted in the docs!
There was a problem hiding this comment.
I could add this to the renderFile, renderData, and renderGraphql. Do you think this would be beneficial? I'm assuming that with a File or Data you could feed in a proper JSON format.
There was a problem hiding this comment.
Hmm, yeah I feel like it couldn't hurt to do so. I can see a situation where someone might want to run a transform before the json parse in any of those situations. To abstract it out a little more, I might just add it in one place though. Here's what I thought when I looked at the code:
- Remove the json.parse from the direct
renderXmethods - Add back the json.parse a single time in the run function, but only run the parse if the data is a string
- Before that json.parse run the transformRaw function once 😁 - this way it wouldnt even need to be abstracted as a function, could just run the ternary inline.
Does that make sense?
There was a problem hiding this comment.
Just saw this. Yeah that makes sense. I'll give it a shot. My latest commit takes care of that ternary and I've got tests that work for all the methods. Right now I'm just calling the transformRaw function in each method, but I think your suggestion is much more efficient and elegant :)
lib/index.js
Outdated
| function transformRaw (obj, data) { | ||
| if (!obj.transformRaw) { return data } | ||
| return obj.transformRaw(data) | ||
| } |
There was a problem hiding this comment.
This can be obj.transformRaw ? obj.transformRaw(data) : data
There was a problem hiding this comment.
Good call here. I will implement this change.
| t.is(out.trim(), '<p>true</p>') | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Thanks for adding a test here, makes a big difference!
There was a problem hiding this comment.
Yep no problem. It's always super easy to add tests when the framework is already there and done nicely.
lib/index.js
Outdated
| @@ -98,16 +98,16 @@ function run (compiler, compilation, done) { | |||
| // Below are the methods we use to resolve data from each type. Very | |||
| // straightforward, really. | |||
| function renderData (obj) { | |||
| return obj.data | |||
| return transformRaw(obj, obj.data) | |||
There was a problem hiding this comment.
I don't think we need this here right?
There was a problem hiding this comment.
Ah yeah I will go ahead and remove that. I was thinking if you wanted to do a transform on the data for some reason you want it there, but the regular transform function would be able to do that so it is redundant.
|
Sweet, this is a great addition. Thanks Drew! |
No description provided.