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 crypto statement to fix vite issue #1652

Merged
merged 1 commit into from
May 15, 2024
Merged

Update crypto statement to fix vite issue #1652

merged 1 commit into from
May 15, 2024

Conversation

brunoocasali
Copy link
Member

@brunoocasali brunoocasali commented May 14, 2024

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 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(..).

src/token.ts Outdated Show resolved Hide resolved
@brunoocasali brunoocasali force-pushed the crypto-fix branch 2 times, most recently from 27480d2 to df8e9dd Compare May 14, 2024 15:52
Copy link

codecov bot commented May 14, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 97.43%. Comparing base (2f37b74) to head (e0b1e71).
Report is 2 commits behind head on main.

Files Patch % Lines
src/clients/client.ts 0.00% 2 Missing ⚠️
src/clients/node-client.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1652      +/-   ##
==========================================
- Coverage   97.54%   97.43%   -0.12%     
==========================================
  Files          22       22              
  Lines         857      858       +1     
  Branches       93       93              
==========================================
  Hits          836      836              
- Misses         20       22       +2     
+ Partials        1        0       -1     

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

@brunoocasali
Copy link
Member Author

I tried this branch with the repo to reproduce the issue https://github.com/flexchar/meili-issue-1299 and it works!

Copy link

@Tragio Tragio left a comment

Choose a reason for hiding this comment

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

Thank you for taking in consideration my comment. I tested on my project and it worked perfectly. Awesome work. ❤️

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.

bors merge

@curquiza curquiza added the maintenance Issue about maintenance (CI, tests, refacto...) label May 15, 2024
Copy link
Contributor

meili-bors bot commented May 15, 2024

@meili-bors meili-bors bot merged commit 9639f78 into main May 15, 2024
5 of 7 checks passed
@meili-bors meili-bors bot deleted the crypto-fix branch May 15, 2024 09:13
meili-bors bot added a commit that referenced this pull request May 15, 2024
1653: Update version for the next release (v0.40.0) r=curquiza a=meili-bot

- Fix the issue introduced in the v0.39 that affected vite apps #1652 `@brunoocasali` 
- Now to use the `generateTenantToken` you should use it with `await`:
  before:
  ```js
  const token = client.generateTenantToken(apiKeyUid, searchRules, {
      apiKey: apiKey,
      expiresAt: expiresAt,
    })
  ```
  after:
  ```js
  const token = await client.generateTenantToken(apiKeyUid, searchRules, {
      apiKey: apiKey,
      expiresAt: expiresAt,
    })
  ```


Co-authored-by: meili-bot <[email protected]>
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.

3 participants