Skip to content

Conversation

@bb
Copy link
Contributor

@bb bb commented Nov 12, 2020

This is a rough conversion of the project to TypeScript. With rough I mean some usage of any here and there, some ad-hoc type definitions and many eslint rules disabled. This fixes #118 and #121. It also sneaks in the fix for Contributing Docs (same as meilisearch/meilisearch-js#687).

Everything should work at least as good as before. It still uses the default export (#122) because the way it is written now, it can be used in another TypeScript project.

@bb
Copy link
Contributor Author

bb commented Nov 12, 2020

The failing integration-test is because meilisearch/meilisearch-js#688 is not yet released.

If you check out meilisearch-js locally and use it by running yarn link in meilisearch-js and yarn link meilisearch in instant-meilisearch, it will work.

@bors
Copy link
Contributor

bors bot commented Nov 13, 2020

🔒 Permission denied

Existing reviewers: click here to make bb a reviewer

@curquiza
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Nov 13, 2020

try

Merge conflict.

@bb bb force-pushed the convert-to-typescript branch from 8d271e1 to b98391f Compare November 13, 2020 10:43
@curquiza curquiza changed the base branch from master to typescript-dev November 16, 2020 18:23
@curquiza
Copy link
Member

curquiza commented Nov 16, 2020

Hello @bb!
Thanks a lot for this work! This is a huge start to move this project to typescript!!
Because it might need some improvements, we created a typescript-dev branch and we will merge your work into this branch as a first step 🙂
@bidoubiwa and I will review your PR and will continue your work on the typescript-dev branch. When all will be ready, we will merge typescript-branch into master.
Thanks again for everything!!! 🚀

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Thanks for this huge work! 😁 We are waiting for the @bidoubiwa's review.

A small change maybe if you have time @bb: could you remove the changes in the CONTRIBUTING.md? It's not related to the typescript work.

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this work! I see you took the configuration of meilisearch-js. Can I ask you if you think some things should be improved in the typescript configuration? In order to know on what to work.

I asked you some questions I hope they are relevant

@bb
Copy link
Contributor Author

bb commented Nov 19, 2020

A small change maybe if you have time @bb: could you remove the changes in the CONTRIBUTING.md? It's not related to the typescript work.

Good point. Sorry for mixing this stuff. I now committed the contributing file from current master on top, so all should be fine again.

@bb
Copy link
Contributor Author

bb commented Nov 19, 2020

Thanks a lot for this work! I see you took the configuration of meilisearch-js. Can I ask you if you think some things should be improved in the typescript configuration? In order to know on what to work.

I thought it would be a good idea to have both projects not diverge too much, so I copied some configs from there. I think the config itself is fine.

Things to improve in general:

  • there's one Todo comment left (index.ts line 14)
  • there are quite a few any type annotations, esp. in InstantMeiliSearchInstance
    • an especially nasty one is (result[key] as any) in format.ts. I didn't find a good solution within a few minutes of googling, so I left it as is.
  • there are a few foo as Bar casts, which are probably also a code smell.
    • e.g. the Object.keys(object) as K[]) in utils.ts line 9
  • It would probably be nice to enable generic Parametrization of the Index's structure (which currently uses meiliSearchResponse: SearchResponse<any, any>)

As you can see, it's all not critical to fix but also not a great clean state yet.

Does this overview help as a starter?

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

I also realize we have to find a way to activate the "auto-reload/rebuild" for the playgrounds because of this typescript integration, otherwise, it will be tedious to develop with them.

Thanks for your work @bb, there is only a small git conflict (I made the typescript-dev breach up-to-date with master).
Afterward, I'm going to merge your PR. @bidoubiwa will continue your work. She's on vacations right now and has emergency tasks on her todo list before doing this one, but be sure she's looking forward to working on it 😉
And thanks for taking the time to answer our questions and to suggest improvements! 🚀

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Thanks for the amazing work

bb added 2 commits December 8, 2020 16:17
@bb
Copy link
Contributor Author

bb commented Dec 8, 2020

I just fixed the merge conflict in package.json and modified the new vanilla-js taks to include the build step directly in the Github UI, but it looks like this was not enough. Will check locally now.

@bb
Copy link
Contributor Author

bb commented Dec 8, 2020

I resorted to an entry in the ignore file to make the linter pass with the TypeScript base settings and the new vanilla-js parts. Not perfect, but better than failing checks.

@curquiza
Copy link
Member

curquiza commented Dec 9, 2020

Merging then!! Thanks a lot @bb for all your involvement!! 🎉

@curquiza curquiza merged commit 7fcdbba into meilisearch:typescript-dev Dec 9, 2020
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.

Usage with TypeScript

3 participants