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 all the things to Typescript #3520

Closed
wants to merge 5 commits into from

Conversation

cellog
Copy link
Contributor

@cellog cellog commented Aug 19, 2019

This is an incremental step. It converts the existing source to use Typescript, but using index.d.ts as the source for all types. I did it this way because the first branch I made had such extensive changes to the files, it assumed they were entirely new, and we lost the granular diffs this PR provides.

A follow-up PR would then begin extracting the types from index.d.ts gradually, until done, and then the last PR would be the configuration changes to tsconfig.ts in both / and tests/typescript. In / this will be turning on .d.ts generation, and in tests/typescript it will be changing the base to point at src instead of index.d.ts

This PR needs to be rebased after #3518 and #3519 are mergedEDIT: good to go!

@cellog
Copy link
Contributor Author

cellog commented Aug 23, 2019

ok @timdorr this is ready for review. If it is too complex, I'm happy to slice and dice to bring each file individually into typescript-land

@cellog
Copy link
Contributor Author

cellog commented Aug 29, 2019

@timdorr ping again, wondering how you'd like to proceed?

DeepPartial,
StoreCreator,
StoreEnhancer
} from '..'
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 just do an export here since we're just shipping it back out? Actually, can't we do that for everything in this file? I'm guessing there's some silly Babel/Rollup + tree-shaking reason why not, but I figure I should ask :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's complicated - we will export them when merging index.d.ts into each file that it touches. For now, this is simplest.

@timdorr
Copy link
Member

timdorr commented Aug 29, 2019

I think we can do this in a big-bang. I'm also OK with there being broken tests in the ts-conversion branch temporarily. As long as it doesn't end up on master in a broken state, we're all good.

@cellog
Copy link
Contributor Author

cellog commented Aug 29, 2019

I think we can do this in a big-bang. I'm also OK with there being broken tests in the ts-conversion branch temporarily. As long as it doesn't end up on master in a broken state, we're all good.

This PR has some issues - I've learned a bit about TS, so I'm splitting them out into separate ones

@timdorr
Copy link
Member

timdorr commented Aug 29, 2019

To add some clarity: We can either remove workarounds for broken tests and leave them temporarily broken, or just annotate that something is a workaround that is to be replaced later (maybe as a // TODO comment with an eslint rule to fail those until removed).

Again, broken is fine while we're in-progress on this stuff.

@cellog
Copy link
Contributor Author

cellog commented Aug 29, 2019

To add some clarity: We can either remove workarounds for broken tests and leave them temporarily broken, or just annotate that something is a workaround that is to be replaced later (maybe as a // TODO comment with an eslint rule to fail those until removed).

Again, broken is fine while we're in-progress on this stuff.

I understand. However, because it is so difficult to debug broken types, I don't think it is as useful to break a whole bunch then try to fix them. I'm confident the work I'm doing now will be both correct and easier to evaluate.

@cellog
Copy link
Contributor Author

cellog commented Aug 29, 2019

closing in favor of the smaller PRs

@cellog cellog closed this Aug 29, 2019
@cellog cellog mentioned this pull request Aug 29, 2019
@timdorr
Copy link
Member

timdorr commented Aug 29, 2019

OK, I'm good with that approach too!

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