Skip to content

fix(geojson): correct array precedence in the generated codegen type#3089

Open
ifeanyi-ugwu wants to merge 1 commit into
graphql-hive:masterfrom
ifeanyi-ugwu:fix/geojson-codegen-type
Open

fix(geojson): correct array precedence in the generated codegen type#3089
ifeanyi-ugwu wants to merge 1 commit into
graphql-hive:masterfrom
ifeanyi-ugwu:fix/geojson-codegen-type

Conversation

@ifeanyi-ugwu

Copy link
Copy Markdown
Contributor

generateGeoJSONType() builds the GeoJSON codegenScalarType by interpolating the Position type into ${Position}[]. Because the postfix [] binds tighter than | in TypeScript, the unparenthesized union

[number, number] | [number, number, number][]

parses as "a 2-tuple or an array of 3-tuples" rather than "an array of Positions". The generated codegen type is therefore wrong for every geometry whose coordinates are arrays of positions — MultiPoint, LineString, MultiLineString, Polygon and MultiPolygon — rejecting valid coordinates like [[1, 2], [3, 4]] while accepting a bare [1, 2].

Parenthesizing the Position union at its source corrects all arrayed usages:

([number, number] | [number, number, number])[]

A regression test asserts the emitted type groups the union before applying [].

generatePositionType() returned an unparenthesized union, so arrayed
usages (`${generatePositionType()}[]`) applied `[]` to only the second
member: `[number, number] | [number, number, number][]` instead of
`([number, number] | [number, number, number])[]`. The codegenScalarType
string emitted to GraphQL Codegen consumers was therefore wrong for
MultiPoint, LineString, MultiLineString, Polygon and MultiPolygon (every
geometry whose coordinates are arrays of positions).

@ardatan ardatan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this right way to reproduce and test this?
Maybe there should be a better test suite to check if the generated types work with your use case?

@ifeanyi-ugwu

Copy link
Copy Markdown
Contributor Author

Is this right way to reproduce and test this? Maybe there should be a better test suite to check if the generated types work with your use case?

You're right, the test is weak — a drop-in. I found this while I was poking around codegen-test/, the dormant type-generation harness, and the real check belongs there. I kept it out of this PR because it wires into CI (the root test script) and shouldn't ride along with a one-line fix.

I've opened #3092 for it and dropped the string assertion here.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants