Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ship Typescript types for Matrix APIs in @matrix-org/spec #1766

Closed
wants to merge 5 commits into from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Mar 22, 2024

@t3chguy t3chguy marked this pull request as ready for review March 22, 2024 19:16
@t3chguy t3chguy requested a review from a team as a code owner March 22, 2024 19:16
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I love this idea: really interesting, thanks!

But I'm not entirely convinced it belongs in the main matrix-spec repo. It feels like the thin end of a wedge, where before too long we'll be maintaining bindings for typescript, kotlin, go, rust, and everything else under the sun. My experience suggests that once you go down that path, releases become very fragile (you can bet that at least one of your target platforms will break), and it's also difficult to make incremental releases that fix things in individual target platforms.

Long story short: I feel like this should be in a separate repo, publishing separate artifacts to npm, based on the published OpenAPI documents from the spec, (eg https://spec.matrix.org/v1.10/server-server-api/api.json).

Happy to hear contrary opinions though.

packages/npm/openapi.sh Outdated Show resolved Hide resolved
packages/npm/openapi.sh Show resolved Hide resolved
-o "$FILE"

yarn openapi-typescript "$FILE" --output "$api.d.ts"
# We remove these lines to workaround dodgy types
Copy link
Member

Choose a reason for hiding this comment

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

what sort of dodgy types, out of interest? Is this something that should be fixed in the openapi definitions?

Copy link
Member Author

@t3chguy t3chguy Mar 25, 2024

Choose a reason for hiding this comment

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

The CS well-known response for example had a [key: string]: Record<string, never> | undefined; which conflicted with the other fields on that response which were not of the type Record<string, never>;

Looks like openapi-ts/openapi-typescript#1055 upstream

Copy link
Member

Choose a reason for hiding this comment

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

ok maybe link to the upstream bug in the comment?

BASE_URL="https://spec.matrix.org/$SPEC_DIR"

for api in application-service client-server push-gateway server-server; do
FILE="$api-api.json"
Copy link
Member

Choose a reason for hiding this comment

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

can we give this a better name? And a comment?

Suggested change
FILE="$api-api.json"
# Combine the separate OpenAPI definition files into one document
combined_doc="${api}-api.json"

@t3chguy
Copy link
Member Author

t3chguy commented Mar 25, 2024

you can bet that at least one of your target platforms will break

The npm releases have been broken for the last N spec releases anyway, I've been handling them manually

I feel like this should be in a separate repo

Happy to move it but I won't be maintaining multiple npm packages so I'd be taking the current package name with me

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@richvdh richvdh requested a review from a team March 25, 2024 11:28
@richvdh
Copy link
Member

richvdh commented Mar 25, 2024

Happy to move it but I won't be maintaining multiple npm packages so I'd be taking the current package name with me

Yup, I (and other members of the SCT) think it makes sense to have a single npm package. Presumably you'd have to pull sas-emoji.json over HTTP from github during the build process, or something?

Sounds good, anyway. If you'd like to make this happen, it would be great!

@t3chguy
Copy link
Member Author

t3chguy commented Mar 25, 2024

Presumably you'd have to pull sas-emoji.json over HTTP from github during the build process, or something?

Yup, it'd also have to be manual given you can't trigger off release events on other repos.

@t3chguy
Copy link
Member Author

t3chguy commented Mar 25, 2024

Replaced by https://github.com/t3chguy/matrix-spec-npm

@t3chguy t3chguy closed this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants