Skip to content

Conversation

@AJRdev
Copy link
Collaborator

@AJRdev AJRdev commented Mar 1, 2019

Description

So here is my first draft of a full typescript migration regarding the issue #142

I still have a couple of tests that are failing to fix but all the migration from Flow is pretty done 😃

I am still improving some stuff and I will have a couple of questions & suggestions on the actual code that raised while typing everything with pretty strict ts-lint rules.

These issues should be fixed by this PR as well : #125 #86

To-do list :

  • Replace Flow typing by Typescript typing in all files
  • Remove Babel & Flow tooling and replacing it by tsc
  • Fix unit tests
  • Fix missing typescript types
  • Check null & undefined cases
  • Suggest some code improvements
  • Try migrate to eslint from tslint (@nodkz)
  • Add prettier (@nodkz)
  • Improve tsconfig (@nodkz)
  • Add flow typing generation at build process (@nodkz)
  • Check debug with port prefix Mongo[${data.port}] in MongoInstance
  • Add new unit tests (needs @nodkz validation)
  • Remove TODOs from PR (@AJRdev )

@AJRdev AJRdev changed the title WIP : Typescript migration https://github.com/nodkz/mongodb-memory-server/issues/142 WIP : Typescript migration Mar 1, 2019
@AJRdev AJRdev self-assigned this Mar 1, 2019
@AJRdev AJRdev force-pushed the typescript-refacto branch from 4b31a07 to 553bda5 Compare March 3, 2019 17:43
@AJRdev AJRdev requested a review from nodkz March 3, 2019 19:33
@AJRdev AJRdev marked this pull request as ready for review March 3, 2019 19:33
@AJRdev
Copy link
Collaborator Author

AJRdev commented Mar 3, 2019

@nodkz I finally fixed the last tests failing in CI due to changes regarding types that I've done.

I have some questions and suggestions for you that I will comment on the code of this PR, when you have some time I'd love to have your feedback and of course your global feedback on this migration 😉

While going through the code I also thought of adding some more tests to the existing test suites, should I make them in another PR or should I add them directly in this PR ?

@nodkz
Copy link
Collaborator

nodkz commented Mar 4, 2019

@AJRdev Did you already migrate your internal projects to eslint from tslint?

Eslint blog post: https://eslint.org/blog/2019/01/future-typescript-eslint
TypeScript Roadmap: microsoft/TypeScript#29288

screen shot 2019-03-04 at 9 03 40 pm

@nodkz
Copy link
Collaborator

nodkz commented Mar 4, 2019

@nodkz I finally fixed the last tests failing in CI due to changes regarding types that I've done.

I have some questions and suggestions for you that I will comment on the code of this PR, when you have some time I'd love to have your feedback and of course your global feedback on this migration 😉

Amazing work!!! 👍👍👍

While going through the code I also thought of adding some more tests to the existing test suites, should I make them in another PR or should I add them directly in this PR ?

You may add tests to this PR.


We will publish a new major version after making all changes.

PS. I'll add some todos assigned to me in the first message.

@AJRdev
Copy link
Collaborator Author

AJRdev commented Mar 4, 2019

@AJRdev Did you already migrate your internal projects to eslint from tslint?

Eslint blog post: https://eslint.org/blog/2019/01/future-typescript-eslint
TypeScript Roadmap: microsoft/TypeScript#29288

@nodkz Not really, first time I'm reading this post about the Typescript team going for ESLint instead of TSLint !
Regarding this post, are you suggesting that I remove all the TSLint tooling in this PR in favor of ESLint ?

From this post of the ESLint team (https://eslint.org/blog/2019/01/future-typescript-eslint), this repo seems to centralize all the packages needed to enable a full Typescript support in ESLint : https://github.com/typescript-eslint/typescript-eslint


Amazing work!!! 👍👍👍

You're welcome, I'm happy to help this package grow 😃

You may add tests to this PR.

Ok I'll add them ASAP.

We will publish a new major version after making all changes.

PS.I'll add some todos assigned to me in the first message.

Ok perfect 👍

@AJRdev
Copy link
Collaborator Author

AJRdev commented Mar 5, 2019

What editor do you use? VSCode?
Do you have installed this plugin https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint

It should catch all errors in opened files.

@nodkz I'm actually using WebStorm because I'm bit more used to the shortcuts, but I'll switch to VSCode for this project to enable a bit more easily the tooling around the project 😉

@AJRdev AJRdev force-pushed the typescript-refacto branch from 901c146 to b5f629a Compare March 7, 2019 12:21
@AJRdev AJRdev force-pushed the typescript-refacto branch from b5f629a to c362ab7 Compare March 7, 2019 12:27
@nodkz
Copy link
Collaborator

nodkz commented Mar 9, 2019

@AJRdev what new test I should validate?
These c362ab7 or other tests?

PS. I'm finished and ready to publish a new 4.0.0 version 😉
PSS. Ping me when you'll make the last changes and cleanups.

@AJRdev
Copy link
Collaborator Author

AJRdev commented Mar 9, 2019

@AJRdev what new test I should validate?
These c362ab7 or other tests?

PS. I'm finished and ready to publish a new 4.0.0 version 😉
PSS. Ping me when you'll make the last changes and cleanups.

@nodkz Yes exactly those tests, if they seems good to you 😉
Ok I will do the last cleanup asap !

@AJRdev
Copy link
Collaborator Author

AJRdev commented Mar 9, 2019

@nodkz Last cleanup and changes done, looks to good to me for release 😈

@nodkz
Copy link
Collaborator

nodkz commented Mar 9, 2019

Great! 👍
Just bathe the child and in an hour or two will publish.

BREAKING CHANGE: migrate codebase from Flowtype to TypeScript
@nodkz nodkz merged commit a5eeeea into master Mar 9, 2019
@nodkz
Copy link
Collaborator

nodkz commented Mar 9, 2019

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nodkz nodkz added the released Pull Request released | Issue is fixed label Mar 9, 2019
@AJRdev AJRdev changed the title WIP : Typescript migration Typescript migration Mar 9, 2019
@AJRdev
Copy link
Collaborator Author

AJRdev commented Mar 9, 2019

@nodkz Thanks for the help, review, merge and release ! 😄

PS : Maybe I should've squashed the commits a bit before merging in master 🤕

@nodkz
Copy link
Collaborator

nodkz commented Mar 9, 2019

PS : Maybe I should've squashed the commits a bit before merging in master 🤕

I did it on purpose of too big changes about 11k lines. So I decided to merge them as is. It will be easy to review for the community, commit by commit. But cons are bad commit messages, but honestly not so bad.

After a month or two the jump to the second page. So don't worry 😉

@AJRdev
Copy link
Collaborator Author

AJRdev commented Mar 9, 2019

@nodkz Fair enough 😉
But it seems the CI coverage job is not available anymore since the merge of this PR, is it normal ?

Now
image

Before
image

@nodkz
Copy link
Collaborator

nodkz commented Mar 12, 2019

@AJRdev found the problem in package.json with CI coverage.
Hope it will be fixed for new PRs

@nodkz nodkz deleted the typescript-refacto branch December 26, 2019 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement released Pull Request released | Issue is fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants