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

Migrate source code to TypeScript [WIP] #128

Closed
wants to merge 2 commits into from

Conversation

urish
Copy link

@urish urish commented Mar 23, 2018

  • add tsconfig.json
  • rename source files .js → .ts
  • install types for 3rd party dependencies
  • configure build tools for typescript (e.g. jest, rollup, lint-staged, prettier)
  • tests pass
  • build passes
  • noImplicitAny (typewiz)

@coveralls
Copy link

coveralls commented Mar 23, 2018

Coverage Status

Coverage increased (+0.2%) to 96.512% when pulling 5747829 on urish:typescript into 6e60eb5 on mweststrate:master.

@urish
Copy link
Author

urish commented Mar 23, 2018

@mweststrate can you have a quick look (especially on the few changes introduced in d08cf3a) and let me know if so far it seems good before I go on and start adding types?

Also, are you okay with removing tests/flow/ts.ts? I don't think we need it anymore as the types will be embedded in the source code and verified by ts and by the tests.

@@ -14,7 +14,7 @@ import {produceEs5} from "./es5"
* @param {Function} producer - function that receives a proxy of the base state as first argument and which can be freely modified
* @returns {any} a new state, or the base state if nothing was modified
*/
export default function produce(baseState, producer) {
export default function produce(baseState, producer?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these parameters have types defined?

Copy link
Author

Choose a reason for hiding this comment

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

Eventually yes. I think the reason I added the question mark was to try to make __tests__/flow/ts.ts happier

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question mark is correct though, produce can be invoked with one argument, basically there are three forms:

produce(baseState, producer) => nextState
produce(producer) => baseState => nextState // curried form
produce(producer, initialState) => baseState => nextState // curried form with default state

@mweststrate
Copy link
Collaborator

@urish looking good so far

@mweststrate
Copy link
Collaborator

@urish preferrably leave the ts.ts, it is not run, but used to verify true negative type check errors

@urish
Copy link
Author

urish commented Mar 27, 2018

@mweststrate cool, I'm on it. I created some initial jest integration for TypeWiz, though hitting some strange issues when running the instrumented test code - I will have to dig in further and isolate them. It could be that Proxies pose some challenges for TypeWiz, we will find out soon :)

@urish
Copy link
Author

urish commented Mar 31, 2018

Update: seems like the Proxies are indeed giving TypeWiz a hard time - the only way I could get all of the tests to pass with TypeWiz was by removing the code that tries to detect the shapes of arrays and object literals. Otherwise, we get all sort of infinite loops and other fun stuff.

Here are the stats I gathered so far:

  • "Stock" TypeWiz - 158 tests fail (66%), tests run for 110 seconds ❌
  • TypeWiz without the object literal shape detection code - 44 tests fail (18%) ❌
  • TypeWiz without the array types detection code - 157 tests fail (65%) ❌
  • TypeWiz without both array / object type detection code - all tests pass in 13.6 seconds ☺️

Another challenge is collecting the type information from the test cases when they finish running. I haven't found any straightforward way to do it through the jest plugin, so right now I'm simply adding the following code at the beginning of each test file:

afterAll(() => {
    require("fs").writeFileSync(
        "frozen-types.json",
        JSON.stringify((global as any).$_$twiz.get())
    )
})

it gets the job done, but then I will still have to merge the collected type info from the different test suites.

I am not sure how useful this would be without Array / Object type inference - I will give it another try tomorrow, and then I hope to share another report here.

- renamed source files .js -> .ts
- install types for deep-freeze, lodash.clonedeep
- install ts-jest, rollup-plugin-typescript2
@mweststrate
Copy link
Collaborator

Closing for inactivity, and since this lib doesn't require much active development TS is not that mandatory

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.

4 participants