swift: Fix initializers with field named self

[lilyball]

When a field is named self, escaping it with backticks (e.g. `self`) when used as a method parameter is not sufficient, as Swift interprets the escaped identifier the same as the keyword. Instead we need to synthesize a safe internal parameter name. This PR picks _self by default, but checks for conflicts and will go with _self_, _self__, etc. until it finds a unique name.

I've updated all of the logic I can find that emits initializers to do this, but my tests only cover some of this as I'm not sure when the other code is used.

This PR also fixes a bug where leading/trailing underscores were being stripped, so e.g. a field named _foo would show up as just foo in the generated code. This is a breaking change, which but it's unlikely to affect many people.

Fixes #1530.

TODO:

• 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

[lilyball]

CC @designatednerd

BTW I have no idea how to run a development build of apollo against my production codebase, which I'd really like to do because I think this is the last issue preventing me from using fields named self and I'd really like to find out without waiting for a new version to be published.

[lilyball]

I'm leaving on vacation tomorrow so I'm definitely not going to have a chance to test this against my production codebase. That shouldn't stop this PR from being reviewed and hopefully merged though.

[designatednerd]

Couple questions for ya @lilyball - would also appreciate if you could rebase when you have a moment. I know you're on vacation so no rush.

[designatednerd]

So am I correct in understanding that makeUniqueName is necessary in case there are properties self and _self on something, since just appending an underscore would cause a conflict?

[lilyball]

In case there’s a property _self. This code only runs if there’s a property named self since that’s what we’re trying to modify (self isn’t valid by itself, so we turn it into _self, and then makeUniqueName ensures we don’t collide with an existing property _self).

I could also have done makeUniqueName(${propertyName}_, properties) instead but I prefer the aesthetic of using _self instead of self_ in the common case. I admit that _self_ looks weird, but that should be vanishingly rare unless someone is being silly.

That said, if you’d prefer to have it start as self_ I won’t object.

[designatednerd]

Was mostly trying to double-check why this had to exist - it'd need to exist even if we went with the self_ method, though. I think using the underscore-as-prefix works better with *OS conventions, so nbd.

[designatednerd]

So it seems like the goal here is to have different behavior (ie, preserve leading and trailing underscores) than the change-case package - why not make that clear by using different function names rather than essentially overriding the functions?

Alternately, would it be preferable to add a preserveLeadingAndTrailingUnderscores parameter available on the underlying functions so other packages could make use of it if needed?

[lilyball]

The underlying functions come from an external dependency, which is a weird constellation of dependencies for different modifications. I was originally going to try and submit a change upstream but it turns out the dependencies all work by first converting the input into space-separated words, e.g. fooBarBaz turns into foo bar baz, and I wasn’t sure if I could make the case for _fooBar to become _foo bar. Personally I think it would make sense. I also didn’t want to block on the dependency making any changes.

I could rename this but this new function is how I think camelCase should work so I’d rather just hide the original and use this.

That said, I don’t know if this new function would be useful outside of apollo-codegen-swift. If other packages import the same camelCase dependency maybe they’d want this new behavior too? I figured we can get it into swift first and then consider moving it into a shared component if other components want it.

[designatednerd]

Fair enough on not wanting to futz with the upstream dependency.

I'm marginally in agreement about "This is how it should work" but I think if we're making changes to what happens under the hood, it should have a different name to make that clear.

[lilyball]

It should only have a different name if there’s a common agreement that something called camelCase should turn _foo into foo. I was quite surprised to find that was the existing behavior.

[designatednerd]

I disagree with that - I think it should have a different name if expected behavior is different than the imported lib's expected behavior, even if you think the imported lib is wrong (unless you've made a PR to fix the imported lib's behavior).

Again, I agree with your decision to not bother messing with the imported lib, but I think we should at least be clear that the expected behavior is different by explicitly naming our methods differently.

[lilyball]

The imported lib is just an implementation detail though. Callers of this function shouldn’t even care that the function relies on an imported lib to work. I’m tempted to just remove the imported lib entirely except I don’t want to risk introducing any behavioral changes (e.g. I think camelCase(“FOO_bar”) will turn into ”fooBar”, which I don’t actually agree with, but I don’t want to make that kind of behavioral change without intent).

[designatednerd]

IMO if it's the same name as the thing being imported, it quacks like an override to me 🦆.

Also, while the comments state that this preserves leading/trailing underscores, but it doesn't state that this is specifically why we're doing this "override". For future maintainers I'd prefer this have a different name for the extra clarity.

[lilyball]

I still don't understand how changing the name of this adds clarity. The "change-case" package does not provide a canonical definition of going between snake_case and camelCase, it's merely an implementation. The fact that our camelCase uses the change-case version internally is literally just an implementation detail, one that I wouldn't mind removing in the future since it seems like an unnecessary dependency. Change the name of this to camelCasePreservingLeadingAndTrailingUnderscores¹ implies the presence of an alternative camelCase function that literally isn't in scope (the imported function is now called _camelCase).

¹I really have no idea what you're suggesting we change it to anyway.

[lilyball]

I guess to make my position more clear, since I feel like we're going in circles: To be an "override" it has to be overriding something that a naïve reader would expect. For example, if I'm overriding something from the JS standard library, or if I'm using a well-known framework but overriding some of its behavior, that's an override. But in this case the fact that we're using the change-case package isn't something a naïve reader would expect and therefore we aren't "overriding" it, it's merely just an implementation detail.

[lilyball]

I’ll rebase this either tonight or tomorrow. I believe any maintainers can edit this PR if they want to rebase it first given the trivial conflict fix.

[designatednerd]

no rush!

[lilyball]

Rebased

[designatednerd]

@lilyball Were you able to test this against the code base which was having issues?

[lilyball]

Just tested, this PR doesn't change the code generation for my existing project. About to test removing some of the selfView: self aliases and seeing if that now works.

[lilyball]

I only removed one of the aliases, but that should be sufficient to test, and it worked. I can now have a GraphQL query that looks like

query ListRitualTokens($channelId: ID!) {
    user(id: $channelId) {
        self {
            ritualTokens {
                ...RitualTokenFragment
            }
        }
    }
}

[designatednerd]

Cool, I'll get one of the JS devs to give it a once-over and then I think we're good to go!

[lilyball]

What's the status of this?

[designatednerd]

Lemme poke the JS devs - do you mind doing a quick conflict fix on the changelog?

[lilyball]

PR updated to fix changelog