Skip to content

Ensure that arrays are properly supported #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

Sytten
Copy link
Member

@Sytten Sytten commented Nov 12, 2020

The latest release broke the output for arrays (basically all arrays got mapped to an array of arrays). I started by fixing that and then I realized it would require a large rewrite to really support arrays properly. So this doesn't introduce a breaking change per the interface, but it does break the contract of rewriteResponse. I added test cases so this cannot happen again.

@Sytten
Copy link
Member Author

Sytten commented Nov 12, 2020

@chanind @gregbty Please review that this doesnt break your intended workflow for the field name rewrite.


// Extract the position
if (Array.isArray(element)) {
element = element[index!] || null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the ! for?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (response === null || typeof response !== 'object') return response;

// Extract the key
let element = response[key];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting a linting error here, this should be const

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right can you fix it since the PR is merge?


return newResults;
return curResults.reduce(
(reducedResults, _, index) => callback(reducedResults, curPathElm, index),
Copy link
Collaborator

@chanind chanind Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I see what you're doing here. We don't have enough info to rewrite child array just on the parent object and the key. I think this is the best fix we can do while keeping the style as close to the current style as possible. 👍

@@ -3,7 +3,7 @@ import ScalarFieldToObjectFieldRewriter from '../../src/rewriters/ScalarFieldToO
import { gqlFmt } from '../testUtils';

describe('Rewrite scalar field to be a nested object with a single scalar field', () => {
it('rewrites a scalar field to be an objet field with 1 scalar subfield', () => {
it('rewrites a scalar field to be an object field with 1 scalar subfield', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 thanks for fixing!

@chanind chanind merged commit adae5d5 into graphql-query-rewriter:master Nov 12, 2020
@chanind
Copy link
Collaborator

chanind commented Nov 12, 2020

🎉 This PR is included in version 3.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Sytten
Copy link
Member Author

Sytten commented Nov 12, 2020

@chanind I would change the message in the release to explain the bug for future reference.

@Sytten Sytten deleted the fix/rewrite-response-mapping branch November 12, 2020 20:32
@chanind
Copy link
Collaborator

chanind commented Nov 12, 2020

crap, I thought that semantic-release would have done that automatically. Let me see if I can change it after the fact...

@chanind
Copy link
Collaborator

chanind commented Nov 12, 2020

👍 OK should be fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants