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

upgrade to typescript #33

Merged
merged 1 commit into from
Dec 13, 2021
Merged

upgrade to typescript #33

merged 1 commit into from
Dec 13, 2021

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Dec 10, 2021

This PR upgrades to typescript and also auto generates typings

@koresar koresar changed the base branch from master to typescript-migration December 13, 2021 00:51
@koresar
Copy link
Collaborator

koresar commented Dec 13, 2021

That is some extraordinary work!

Besides TS rewrite there is a lot to thank you for.
I like how you fixed typos, used mongodb-memory-server, introduced ESLint, code coverage,

I'm going to quickly amend the PR. Few reasons:

  • The README.md states that the new default collection name is "transactions", whereas is must not change. Like ever. :) So, I'm going to fully reverse the README.md changes.
  • README.md would need some info about the TS support. I'll add some.
  • Also we need the full list of all breaking changes.
  • I'll remove console.log statements from the unit tests. Because I like silent tests. No other reasons frankly. :)
  • Also, I'd need to rewrite before() and after() mocha hooks from callbacks to promises. For consistency.
  • Also, prettier is a must in all our projects. I'll have to add it back to devDependencies.
  • Other minor changes I have to do to avoid much breaking changes.

I've changed the base branch for this PR to typescript-migraiton.

@koresar koresar merged commit d80aac7 into flash-oss:typescript-migration Dec 13, 2021
@koresar
Copy link
Collaborator

koresar commented Dec 13, 2021

Hello @Uzlopak

You have been granted Write access to this repo. Feel free to develop and push right to this repo. This would simplify yours and ours workflows.

The branch typescript-migraiton needs a number of improvements. I haven' done any yet because I lack knowledge.

  1. I pulled the typescript-migraiton and ran using node 12:

rimraf package-lock.json node_modules/ && npm i
npm run build

It doesn't build! :(

  1. ESLint configuration is missing.

The npm run lint command(s) does not work. :(

  1. If we are upgrading to mongoose v6, then we should also upgrade to the latest node LTS range. Meaning medici must support node 12, 14, and 16. The minimum supported is v12.

I tried running it in node 12, and it fails. :( Also CI, TS, ESLint and other tools must support node 12 and corresponding ES version.

  1. I couldn't conduct the proper list of all the breaking changes this PR does. Could you please do it?

  2. We must not rename the default collection name from Medici_Transaction. It must stay. There are too many reasons for that.

  3. Prettier should be used to format all the code.

Minor note.

I'm happy to add TS, ESLint, nyc, etc to the project. They are useful.

This package was trying to be minimalistic on the surrounding tooling. That's why it only has mocha and prettier in devDependencies. Because we find that every dependency results in time waste. For example, I could have merged this whole code to the master branch. But it does not work because of some TS/ES/Node/API/etc version mismatch.

Please, checkout flash-oss:typescript-migration branch and push all the new commits directly to that branch.

Again, thank you very much for all that work! Some minor things left. I really hope you can help us resolving them.

@Uzlopak Uzlopak mentioned this pull request Dec 14, 2021
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