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

Convert lib/types dir to TypeScript #794

Merged
merged 22 commits into from
Oct 4, 2021

Conversation

owenpearson
Copy link
Member

Resolves #790

Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

In an ideal world I would have liked to see new API commentary alongside at least some of the modules, types and methods/functions. This could be an opportunity to inject some of that, though I recognise it may not have been a priority for you.

I'm also noticing a lot of variance in terms of indentation. e.g. common/lib/types/stats.ts vs common/lib/util/dataSizeBytes.ts. I've not pulled the code in locally to inspect further but the impression is that some files are using tabs and others using spaces. I assume, therefore, that we're not running any tools over the code in CI that would spot for formatting inconsistencies? I think we should be and these tools should form foundational underpinnings to this work - otherwise we're only going to need to go back over all the files again some time in the future to conform them.

common/types/platform-bufferutils.d.ts Show resolved Hide resolved
common/lib/util/forInOwnNonNullProps.ts Outdated Show resolved Hide resolved
common/lib/util/logger.ts Outdated Show resolved Hide resolved
common/lib/types/stats.ts Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to staging/pull/794/bundle-report September 30, 2021 11:34 Inactive
@owenpearson
Copy link
Member Author

@QuintinWillison The tabs vs spaces stuff should be fixed now.

As for the API commentary that would go on the external type interface (ably.d.ts) and would be unrelated to this work. Maybe one day we can automatically generate the external type interface from the source code but it would be hard to do that without exposing a bunch of internal typings that we wouldn't want users of the library to see.

@owenpearson owenpearson merged commit b903da7 into integration/typescript Oct 4, 2021
@owenpearson owenpearson deleted the feature/types-typescript branch October 4, 2021 14:07
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