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

Update ESLint, Prettier, TypeScript and fix/improve their configuration files #1616

Merged

Conversation

flevi29
Copy link
Collaborator

@flevi29 flevi29 commented Jan 1, 2024

Pull Request

Related issue

Fixes #1615

What does this PR do?

Along with what is already detailed in the related issue, the PR includes a lot of comments about the why and what, most of which should be removed once clarified.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@flevi29
Copy link
Collaborator Author

flevi29 commented Jan 2, 2024

For Node.js 16 and 18 it runs fine for me, looks like it cannot connect to Meilisearch for some reason.

Regarding Node.js 14 the tests fail because the latest @typescript-eslint/eslint-plugin doesn't support this version anymore. We could either install an earlier version of @typescript-eslint/eslint-plugin (which might cause other issues with related package versions) or simply drop testing and officially supporting Node.js 14 (has been end-of-life since 2023-04-30).

@flevi29 flevi29 force-pushed the improve-eslint-prettier-tsconfig branch from c808d06 to bbe4ed1 Compare January 8, 2024 17:02
@flevi29
Copy link
Collaborator Author

flevi29 commented Jan 10, 2024

I might want to revert changes from doing style:fix which runs prettier. Ran prettier for more files than it was run before, and touched some files in tests/env that confuse me very much as to how did they ever work (broken tests), and why they were done the way they are. Also it's pretty hard to review with this many changes.

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.50%. Comparing base (d891f15) to head (9759c35).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1616      +/-   ##
==========================================
- Coverage   97.52%   97.50%   -0.03%     
==========================================
  Files          22       22              
  Lines         850      842       -8     
  Branches      102       86      -16     
==========================================
- Hits          829      821       -8     
  Misses         20       20              
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@flevi29
Copy link
Collaborator Author

flevi29 commented Jan 11, 2024

Okay, now whatever happens is expected. I'm really confused about tests/env/express/public/meilisearch.umd.js, and why all of a sudden everything broke after formatting some files around it. I guess that's just another thing to watch out when improving tests.

@flevi29 flevi29 marked this pull request as ready for review January 17, 2024 08:01
@flevi29
Copy link
Collaborator Author

flevi29 commented Jan 17, 2024

Alright, so this is ready for review, style-check is expected to fail on prettier checking files if they're formatted. Otherwise there would be quite a few more changes.

@curquiza
Copy link
Member

@flevi29
I'm confused, what should we do if the failure are expected? I don't want to (cannot) merge the PR with a CI failure

Shoudn't we adapt our style check?

@flevi29
Copy link
Collaborator Author

flevi29 commented Jan 17, 2024

Ideally someone should review my changes, who understands what is being done. After that I remove all the unnecessary explainer comments and run the formatter, so that there may be no more CI errors (other than perhaps codecov, but that's probably looking at scripts rather than code, so it can be ignored). If no one at meili is around who really understands this stuff, then I guess it's entirely up to you and @brunoocasali if you trust the PR or not. If you don't understand something that I haven't quite explained via comments, ask away.

@curquiza
Copy link
Member

Hello @flevi29 sorry for the delay, we have a lot on our plate currently

@brunoocasali will review it when he will find the time. Currently, it's complex regarding our time, but we don't forget you!

@curquiza curquiza added the maintenance Issue about maintenance (CI, tests, refacto...) label Feb 14, 2024
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

I understood the proposal not to run the changes in the same PR. Thanks for that!

I'm approving this PR. Thanks a lot for your help and sorry for the huge delay!

Comment on lines +53 to +54
"fmt": "prettier -c ./**/*.{js,ts,tsx}",
"fmt:fix": "prettier -w ./**/*.{js,ts,tsx}",
Copy link
Member

Choose a reason for hiding this comment

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

I would just ask to remove this ,tsx because it will prevent running the command later:
[error] No files matching the pattern were found: "./**/*.tsx".

Suggested change
"fmt": "prettier -c ./**/*.{js,ts,tsx}",
"fmt:fix": "prettier -w ./**/*.{js,ts,tsx}",
"fmt": "prettier -c ./**/*.{js,ts}",
"fmt:fix": "prettier -w ./**/*.{js,ts}",

Unless you have a way to fix it without removing!

brunoocasali added a commit that referenced this pull request Apr 3, 2024
brunoocasali added a commit that referenced this pull request Apr 3, 2024
@brunoocasali brunoocasali merged commit 39a874f into meilisearch:main Apr 3, 2024
4 of 6 checks passed
@flevi29 flevi29 deleted the improve-eslint-prettier-tsconfig branch April 4, 2024 15:29
meili-bors bot added a commit that referenced this pull request May 6, 2024
1648: Update version for the next release (v0.39.0) r=brunoocasali a=meili-bot

## 🚀 Enhancements

* feat: hybrid search improvements for v1.8.x (#1647) `@mdubus`
* Add `null` to Embedder type (#1646) `@amit-ksh`
* Add searchCutoffMs index setting (#1643, #1645) `@amit-ksh`
    ```js
    client.index('movies').getSearchCutoffMs()
    client.index('movies').updateSearchCutoffMs(150)
    client.index('movies').resetSearchCutoffMs()
    ```

## ⚙️ Maintenance/misc

* Update ESLint, Prettier, TypeScript and fix/improve their configuration files (#1616) `@flevi29`
* Fix code style after configuration changes (#1638) `@brunoocasali`

Thanks again to `@amit-ksh,` `@brunoocasali,` `@curquiza,` `@flevi29,` `@mdubus,` `@meili-bors[bot]` ! 🎉


Co-authored-by: meili-bot <74670311+meili-bot@users.noreply.github.com>
meili-bors bot added a commit that referenced this pull request May 15, 2024
1652: Update crypto statement to fix vite issue r=curquiza a=brunoocasali

After the introduction of this PR #1616 we had a bunch of issues related to vite configuration on client projects as shown here meilisearch/meilisearch-js-plugins#1299

This PR introduces the rollback needed to fix the consumer's code. If this change is not enough, I will completely reverse the PR 1616.


Edit:

I was able to solve the issue, by using this branch with this [reproduction repo](https://github.com/flexchar/meili-issue-1299) I could build the project and also run the application without any issue.

But this change introduces some breaking changes in the JWT code:

Now it is required to use await: `await client.generateTenantToken(..)` instead of just `client.generateTenantToken(..)`.

Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Issue about maintenance (CI, tests, refacto...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eslint and prettier outdated, messy/faulty configurations, these and typescript too need improvement
3 participants