Skip redundant modifiers

[michaelnisi]

Resolves #1177. The goal is to prevent compiler warnings for redundant access-level modifiers when using --namespace.

It covers queries and fragments, according to basic testing at least, resolving the initial issue. Are there more types affected by this? @martijnwalraven

• Update CHANGELOG.md* with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[trevor-scheer]

Thanks for taking the time to put this together @michaelnisi! The PR is greatly appreciated. I'm going to hand this off to @designatednerd for review since she's our new mobile architect 😄

[designatednerd]

I'm crap at typescript so I'll have to defer to an actual ts developer on that bit.

[designatednerd]

I haven't been able to dig super-deep into what's happening here, but is the adoptedProtocols thing what shoves this into an extension rather than just into the big namespaced enum?

[michaelnisi]

Give me a minute to refresh my memory.

[designatednerd]

@michaelnisi Any recollection?

[michaelnisi]

First thing tomorrow morning.

[michaelnisi]

No, these changes simply skip the public modifier if we are in a namespace, see below. 👇 adoptedProtocols is checked to see if our struct is a fragment.

[michaelnisi]

@designatednerd Lost interest? 🙂

[designatednerd]

Nope lost the changes in my flood of email 😄 will take another look

[michaelnisi]

Rebased and reviewed. These changes simply make the previously static public and final modifiers dynamic to prevent redundancy warnings #1177.

The goal is to skip the public modifier in the namespaced case. It boils down to this 👇

For classes:

const isRedundant = !!namespace;
const modifiers = isRedundant ? ["final"] : ["public", "final"];

For structs:

const isRedundant = adoptedProtocols.includes("GraphQLFragment") && !!namespace;
const modifier = isRedundant ? "" : "public ";

These changes cover queries and fragments.

[designatednerd]

@trevor-scheer I'll leave this to you to review the typescript on Monday but the swift stuff is ready to ship!

[trevor-scheer]

Minor nitpick but TS changes LGTM!

[michaelnisi]

Rebased and ready to go.

However, there’s a new (unrelated?) failing CI check for Node.js 10:

Summary of all failing tests
 FAIL  packages/apollo/src/commands/client/__tests__/generate.test.ts (6.301s)
  ● client:codegen › extracts queries with a custom tagName provided in the config

    TypeError [ERR_INVALID_ARG_TYPE]: The "warning" argument must be one of type Error or string. Received type object

Let me know if this is a new requirement the Swift generator is neglecting and if I can help.

[trevor-scheer]

@michaelnisi those are some flaky tests that we've been tolerating for quite some time now. All was well after a re-run. Thanks for the contribution! 🎉

[gsabran]

This should only apply if the classes and structs are defined within an extension, which I think is equivalent to outputIndividualFiles == true. Else the code generated now looks like

public enum MyNameSpace {
    struct Hero: GraphQLFragment {
        public var name: String
        public var age: Int
    }
}

and the struct is internal.