fix: Escape deprecation messages #2951
Conversation
✅ Deploy Preview for apollo-ios-docs canceled.
|
Sources/ApolloCodegenLib/TemplateString+AvailabilityDeprecated.swift
Outdated
Show resolved
Hide resolved
Sources/ApolloCodegenLib/TemplateString+AvailabilityDeprecated.swift
Outdated
Show resolved
Hide resolved
|
@AnthonyMDev, working through this made me realize I don't think we're generating |
Sources/ApolloCodegenLib/TemplateString+AvailabilityDeprecated.swift
Outdated
Show resolved
Hide resolved
Tests/ApolloCodegenTests/CodeGeneration/Templates/TemplateString_DeprecationMessage_Tests.swift
Outdated
Show resolved
Hide resolved
| // These tests ensure that when given double quotes in SDL the generation of Swift code works as | ||
| // expected from frontend parsing all the way to rendering of attributes, comments andwarnings. | ||
| // There is a test here for all the places that the GraphQL schema supports the @deprecated | ||
| // directive. |
There was a problem hiding this comment.
Thank you for adding these tests. Super critical b/c if we change the back end to SwiftGraphQL someday we want to ensure it still works all the way through!
There was a problem hiding this comment.
These tests don't include the characters \0 (null character), nor \' (single quote) because they're caught as syntax errors by graphql-js.
failed: caught error: "Syntax Error: Invalid character escape sequence: "\0".
failed: caught error: "Syntax Error: Invalid character escape sequence: "'".
I think this is because graphql-js is validating strings against the GraphQL spec definition of allowed escape characters. Those two characters are not in that list.
There was a problem hiding this comment.
@AnthonyMDev similarly graphql-js does not allow \( either so there is no need to check and replace that character. What we spoke about yesterday.
failed: caught error: "Syntax Error: Invalid character escape sequence: "(".
We definitely aren't, but I'm not sure if that's necessary. If the fields themselves have deprecation messages, you're just going to get duplicate warnings. What situation would you need to have a deprecation warning on the initializer itself? |
Consistency. The codegen configuration has an option to generate warnings on deprecation but we're making a decision to only implement them in some places; how should the user know where to expect them? I don't know if it's urgent we add them though. |
|
|
||
| forEach { character in | ||
| switch (character) { | ||
| case "\0": escapedString.append(#"\0"#) |
There was a problem hiding this comment.
I'm open to alternatives that would be more efficient or better performing than this; all the other ways I tried I couldn't use extended string delimiters and using regular string literals causes the escaped characters to be 'processed' instead.
There was a problem hiding this comment.
It feels like there has got to be a better way of doing this, but I'm not sure what that would be honestly, so lets go with it. This is not the most performance sensitive code to be honest.
I wonder if using a dictionary for the find and replaces would be slightly more performant, but if I remember correctly this switch statement should end up compiling into something very similar to that anyways. (I could be totally wrong about that, it's been a long time since I looked into that.)
|
|
||
| forEach { character in | ||
| switch (character) { | ||
| case "\0": escapedString.append(#"\0"#) |
There was a problem hiding this comment.
It feels like there has got to be a better way of doing this, but I'm not sure what that would be honestly, so lets go with it. This is not the most performance sensitive code to be honest.
I wonder if using a dictionary for the find and replaces would be slightly more performant, but if I remember correctly this switch statement should end up compiling into something very similar to that anyways. (I could be totally wrong about that, it's been a long time since I looked into that.)
Tests/ApolloCodegenTests/CodeGeneration/Templates/TemplateString_DeprecationMessage_Tests.swift
Outdated
Show resolved
Hide resolved
Tests/ApolloCodegenTests/CodeGeneration/Templates/TemplateString_DeprecationMessage_Tests.swift
Outdated
Show resolved
Hide resolved
Fixes #2879
#warningand@available) into one extension