feat(rewriter): expose parent object and element path to rewriter #20
Conversation
This changes the implementation of `rewriteResultsAtPath` so that now the parent and path of the element is exposed to `Rewriter`.`rewriteResponse` instead of the element itself. This should support implement field name response rewrites. BREAKING CHANGE: This changes the definition of `Rewriter`.`rewriteResponse`. The `path` references the location of the element while the `response` points to the parent of the element. Combining these will result in the original behavior.
src/RewriteHandler.ts
Outdated
| @@ -72,8 +72,8 @@ export default class RewriteHandler { | |||
| let rewrittenResponse = response; | |||
| this.matches.reverse().forEach(({ rewriter, paths }) => { | |||
| paths.forEach(path => { | |||
| rewrittenResponse = rewriteResultsAtPath(rewrittenResponse, path, responseAtPath => | |||
| rewriter.rewriteResponse(responseAtPath) | |||
| rewrittenResponse = rewriteResultsAtPath(rewrittenResponse, path, (parentResponse, path) => | |||
There was a problem hiding this comment.
nit: calling this path here is confusing since there's already a variable called path in this function. This should probably be added to the linter
There was a problem hiding this comment.
I renamed this to key, hopefully that suffices, based on your other feedback.
src/ast.ts
Outdated
| @@ -261,24 +261,38 @@ interface ResultObj { | |||
| export const rewriteResultsAtPath = ( | |||
| results: ResultObj, | |||
| path: ReadonlyArray<string>, | |||
| callback: (resultsAtPath: any) => any | |||
| callback: (parentResult: any, path: string | number) => any | |||
There was a problem hiding this comment.
nit: could this be called something other than path, since path is used to mean an array of strings in other places in this function? Maybe key or something similar?
There was a problem hiding this comment.
Updated! (Naming is hard.)
| return newResults; | ||
| newResults[curPathElm] = curResults.map(result => | ||
| rewriteResultsAtPath(result, remainingPath, callback) | ||
| ); |
There was a problem hiding this comment.
👍 good refactor, don't know why reduce was used here instead of just a simple map like you wrote.
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Looks like I missed one thing. This doesn't change the behavior of the output of the rewrites. While we can now see the parent object and modify it, those changes are ignored because the caller only maps the result to the specified path/key. Fixing that is a much bigger change because it would fundamentally change how rewrites are done. For now though, I can at least use this to rename paths by looking via a parent rewrite. |
This changes the implementation of
rewriteResultsAtPathso that now the parent and path of the element is exposed toRewriter.rewriteResponseinstead of the element itself. This should allow #14 to modify the response.