Added Fragment Support

[natac13]

Create new tests to verify that the requested fields are included in the $project

This was done by modifying the getSelections() function to use a reduce function over map. This allowed to check if sel.kind === 'FragmentSpread'; if so, perform another recursive call to getSelections A second argument was added to getSelections, which is the fragments of the incoming query.

The tests go 3 levels deep of fragments.

Closes #55

Please note I ran the full suite of test on the branch I used to developed the new feature; there were 6 failed suites with regards to close() not being a function, on unrelated test projects. I switched to the master branch and made sure to do a pull from upstream (your project). However there were still 6 test suites that are failing. Leading me to believe my changes did not cause the failures. :crossed_fingers: :crossed_fingers:

I was not able to figure out why those tests are failing since spinup() in those directories is exporting a close() fn..... Could you help me out with them?

Hope the new fragment support works out!

[arackaf]

This looks great! I'll investigate those tests tomorrow - almost certainly nothing you did.

[arackaf]

Yo - thanks for starting this. I've got this merged into a branch called fragment-support. I fixed a bug or two with your original, added support (and tests) for fragment use in relationships, and also mutations. This may be a premature optimization, but I also ditched you reduce call for a for loop, to remove all those array and map allocations.

All tests are passing now - can you take a look and let me know if you see any problems? If not I'll add a note to the docs, and publish.

Also, feel free to just comment out those 6 failing tests (just don't commit!) Those tests require a Mongo 4 replica set, so I can test transactions, and so I have a hosted Mongo DB for that purpose, and a connection string to it that's not checked in.

[natac13]

Hey Adam. Thanks for helping with the PR, fixing the test and adding a ton more. I have looked over the diff of your fragment-support branch and the one I created; all looks great! Glad I was able to lend the initial hand.

Glad to know about the test only being with regards to transactions.

Lastly I have been working on my own (very simplified) version to support my use case with mongoose. So the original getSelections I wrote that had my think to open a PR looked like the following. My own version and is using objects instead of a Map

function getSelections(fieldNode, fragments) {
  return R.reduce((acc, sel) => {
    // check if the sel.kind is a fragmentSpread
    // if so send the fragment fieldNode to the getSelections recursive fn.
    if (sel.kind === 'FragmentSpread') {
      return {
        ...acc,
        ...getSelections(fragments[sel.name.value], fragments),
      };
    }
    return {
      ...acc,
      [sel.name.value]: R.isNil(sel.selectionSet)
        ? 1
        : { ...getSelections(sel) },
    };
  }, {})(R.pathOr([], ['selectionSet', 'selections'])(fieldNode));
}

PR version

function getSelections(fieldNode, fragments) {
  return new Map(
    fieldNode.selectionSet.selections.reduce((acc, sel) => {
      if (sel.kind === "FragmentSpread") {
        return [...acc, ...getSelections(fragments[sel.name.value], fragments)];
      }
      const y = [
        sel.name.value,
        sel.selectionSet == null ? true : getSelections(sel)
      ];
      return [...acc, y];
    }, [])
  );
 }

This may be a premature optimization, but I also ditched you reduce call for a for loop, to remove all those array and map allocations.

And I see that your version is much cleaner! I tried to make the same modifications to my own code. You upgraded code

function getSelections(fieldNode, fragments, result = new Map([])) {
  for (let sel of fieldNode.selectionSet.selections) {
    if (sel.kind === "FragmentSpread") {
      getSelections(fragments[sel.name.value], fragments, result);
    } else {
      result.set(sel.name.value, sel.selectionSet == null ? true : getSelections(sel, fragments));
    }
  }
  return result;

My own version

function getSelections(fieldNode, fragments, result = {}) {
   for (let sel of R.pathOr([], ['selectionSet', 'selections'])(fieldNode)) {
     if (sel.kind === 'FragmentSpread') {
       getSelections(fragments[sel.name.value], fragments, result);
     } else {
       result[sel.name.value] = R.isNil(sel.selectionSet)
         ? 1
         : { ...getSelections(sel, fragments) };
     }
   }

}

Which works however the Airbnb linting guide I use gives a warning which lead me to this issue airbnb-styleguide issue #1122.

Therefore I tried to write the function to make the linter happy while trying to understand the optimization you have shown me with for...of

function getSelections(fieldNode, fragments, result = {}) {
  R.forEach((sel) => {
    if (sel.kind === 'FragmentSpread') {
      getSelections(fragments[sel.name.value], fragments, result);
    } else {
      result[sel.name.value] = R.isNil(sel.selectionSet)
        ? 1
        : { ...getSelections(sel, fragments) };
    }
  })(R.pathOr([], ['selectionSet', 'selections'])(fieldNode));
  return result;
}

Linter does not like the param re-assign....

If you are at all able, could you help me understand what the best approach is and why? I see that you use for...of in this code base and was just wondering why there is so much of a divide between coders on this topic? Referring to the issue linked.

I, for being completely self taught, favor more readable code over anything. However, there is so much emphasis put on functional purity that I find it hard to balance the two.

Thanks again Adam.

[natac13]

I just did a small test to my reduce version and see that it goes to a recursive depth that the for...of loop or forEach version do not.

Glad you made me aware of this! :+1:

[arackaf]

Hey - I’ll look at those other questions a bit later - for now tho do you have a Twitter handle? Wanna give you some credit for starting the fragment work.

[natac13]

Yep, I followed you. @natac131

[arackaf]

Regarding your other question, I'd urge you to completely ditch the AirBnB style guide / lint rules and never look back.

[natac13]

I'd urge you to completely ditch the AirBnB style guide / lint rules and never look back.

Will do! Is there one that you recommend? Or just use the default eslint:recommended and modify as I see fix?

I read many comments that echo yours. I really appreciate the help and advice.

[arackaf]

For sure - I actually don't bother with eslint. If you're looking for build-time verification of your code, I'd recommend you just use TypeScript. (plenty would disagree with me though)