Open KasMA1990 opened 4 years ago
Hello! The challenge for the generated code is that Elm top-level definitions (functions and values) must start with a lowercase alpha character. Module names and types in Elm must start with a capital alpha character. So elm-graphql
has to normalize these names to be valid in the generated code.
Here is the relevant code for normalizing names:
So it's challenging to find a strategy that works well for that. The underlying philosophy I'm aiming for is to shoot for is:
There are no great options here and no silver bullets (there are problems with any approach). But one strategy could be to take these non-idiomatic names (types are idiomatically capitalized in GraphQL) and generate something that will be less likely to result in name collisions. For example:
type SortBy {
# idiomatic class case name
}
type sortBy {
# non-idiomatic camel case type name
}
We could generate resulting modules like this:
SortBy
-> SortBy.elm
sortBy
-> X_sortBy.elm
Just for completeness, another idea is to normalize all names. That is, start every module name with X_...
so that what comes after the _
can be lowercase or uppercase. However, this seems like a nuclear option in that it's a big breaking change, and it also requires the 99% of cases that are using idiomatic names to pay the penalty just to support the irregular cases in a consistent way.
elm-graphql
could have warnings when there is a collision like the one you described. However, a warning doesn't seem very robust because it doesn't prevent the build and allows you to run the app with a problem that you may not have noticed.
Similar to idea 3, but stop the build. Erroring out when there are collisions is more robust, but also more rigid. If you don't have control over your API, then you're out of luck. So this doesn't seem like a viable approach in itself.
Another approach could be to require a mapping file to map any irregular names. In your case, that might look like a JSON file like this:
irregular-elm-graphql-name-mappings.json
{"typeNames":{"sortBy":"X_sortBy"},
"fieldNames": {}
}
There are some special cases here that would probably deserve errors:
One of the challenges with this approach is that you may run this when there is an API change and not expect a failure. But that might be an acceptable tradeoff because you probably want to stop rather than having a silent conflict like the current state.
It could even allow for custom error messages. This could even just be Elm code that runs with a type signature like:
normalize : String -> Result String String
Or it could take in a custom type which enumerates possibilities, like:
type GraphqlName = Regular String | Irregular IrregularReason String
type IrregularReason = LowerCaseTypeName | ...
I think that Idea 5 may be the best balance between simplicity, robustness, and flexibility. I'd love to hear other peoples thoughts on this. Real world use cases and experiences with GraphQL/elm-graphql would be very useful to hear as well!
I'll gather some feedback here, and then consider implementing Idea 5.
I think idea 5 sounds good. For our use case, we would like to have elm-graphql
fail to build when ambiguous names are present. We wouldn't want mappings for ourselves (since we control the schema, and would rather fix that), but it makes sense for others who don't control their schemas.
In our case, having an explicit "Disallow unidiomatic names" option could be nice, even when there are no ambiguities. We do currently have a few unidiomatic names, and we can't just replace them immediately, because we want to have deprecation periods for any schemas we might change (as our customers use those them). It would be useful to verify that we are not adding new unidiomatic names though, so maybe such an option could require us to add unidiomatic names to the mapping file, to make them explicit.
Such an option would grow elm-graphql
a bit in the direction of linting GraphQL schemas, which might be out of scope for what you want, so feel free to disregard if you don't like it :slightly_smiling_face:
Just got bitten by something similar:
enum Order {
ASC
asc
DESC
desc
}
The lowercase versions exist only for backwards compatibility reasons and are deprecated, but it causes the generation of the invalid elm type:
type Order
= Asc
| Asc
| Desc
| Desc
Could it be simply / at least deduplicated in this instance?
If someone wants to try adding a feature for approach 5, I would welcome a PR there. It may be a little bit involved because it would require passing around the mapping configuration to all the relevant places. I'd be happy to give guidance on that if someone wants to work on that.
We have a GraphQL backend, where one of our developers recently added a
sortBy
type, which caused us some trouble, given that aSortBy
type already existed. The GraphQL standard defines that types must have unique names, but doesn't specify anything about case sensitivity as far as I can tell, so it seems like this is legal.Using elm-graphql to generate our Elm code, it would only generate one
SortBy
module, non-deterministically letting one type overwrite the other. This made the issue go undetected by the developer who introduced the change, since only one of the two types were actually used on our front-end at the time, so the build would be green if the right type was generated.The fix was pretty simple once we knew what the issue was, since we don't want disparate types that are named so closely, so we prefixed the type names with more info. However, it would be nice if elm-graphql could warn or fail completely in this situation, since the generated code is arguably invalid. But if it's troublesome to support, it's also not a huge deal, since the problem was relatively straight forward to deal with 🙂