Skip to content

[Swift Codegen] Adding some explicit type annotations#1638

Merged
designatednerd merged 4 commits intoapollographql:masterfrom
djs-code:djs-code/more-swift-type-annotations
Nov 24, 2019
Merged

[Swift Codegen] Adding some explicit type annotations#1638
designatednerd merged 4 commits intoapollographql:masterfrom
djs-code:djs-code/more-swift-type-annotations

Conversation

@djs-code
Copy link
Copy Markdown

@djs-code djs-code commented Nov 7, 2019

Adding explicit type annotations for operationDefinition, operationName, and possibleTypes to help out Swift compile times.

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

@apollo-cla
Copy link
Copy Markdown

@djs-code: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@djs-code djs-code force-pushed the djs-code/more-swift-type-annotations branch from 94f19c3 to 02a6374 Compare November 7, 2019 23:25
@designatednerd
Copy link
Copy Markdown
Contributor

@djs-code out of curiosity, what's the compile time improvement you're seeing after adding explicit types?

@designatednerd
Copy link
Copy Markdown
Contributor

looks like something in here is failing the linter - @trevorblades or @abernix any rough clue what?

@designatednerd designatednerd added 🐦 component - swift 🤖 component - codegen related to the codegen core packages labels Nov 8, 2019
@djs-code
Copy link
Copy Markdown
Author

djs-code commented Nov 8, 2019

@designatednerd The benchmarking wasn't too extensive, but the handful of files I tested the before/after of yielded a 12~15% improvement.

@designatednerd
Copy link
Copy Markdown
Contributor

designatednerd commented Nov 8, 2019

That's not bad for just throwing in a few types.

Let's see what the JS group have to say when they have a moment about the lint failures, I already kicked the build once and am seeing the same issues - you might be able to trigger the linter to auto-format by saving and running tests with npm test before committing, but that's been suuuuper inconsistent for me.

@designatednerd
Copy link
Copy Markdown
Contributor

Apparently: “Running npm run lint-fix will apply those changes.”

Sent with GitHawk

@djs-code
Copy link
Copy Markdown
Author

djs-code commented Nov 8, 2019

@designatednerd Looks like that did the trick! 🎉

@djs-code
Copy link
Copy Markdown
Author

djs-code commented Nov 8, 2019

...or did I speak too soon? 🤔

@designatednerd
Copy link
Copy Markdown
Contributor

OK kicked the build - the thing that failed won't rerun until after all the other tests are done

Copy link
Copy Markdown
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Good to merge whenever CI gets its shit together

@designatednerd
Copy link
Copy Markdown
Contributor

Tooling team's on it - your code is fine. 😃

@djs-code
Copy link
Copy Markdown
Author

@designatednerd Heya, have we been able to nail down what's going awry here?

@designatednerd
Copy link
Copy Markdown
Contributor

@djs-code Can you try rebasing off master? I believe it may have been fixed there

Dan Stenmark added 4 commits November 21, 2019 10:36
Addiing explicit type annotations for `operationDefinition`, `operationName`, and `possibleTypes` to help out compile times.
@djs-code djs-code force-pushed the djs-code/more-swift-type-annotations branch from 2a39a87 to 1b04d48 Compare November 21, 2019 18:36
@djs-code
Copy link
Copy Markdown
Author

@designatednerd Yup, looks like we're good to go. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐦 component - swift 🤖 component - codegen related to the codegen core packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants