Skip to content

Conversation

Strift
Copy link
Collaborator

@Strift Strift commented Jan 17, 2025

Pull Request

Related issue

Fixes #1360

Supersedes #1362 (thanks to @flevi29 for the help)

What does this PR do?

While working on #1360, I went down the rabbit hole of making instant-meilisearch use ESM for TypeScript, Rollup, and Jest.

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!

@Strift Strift marked this pull request as draft January 17, 2025 04:03
Copy link

changeset-bot bot commented Jan 17, 2025

⚠️ No Changeset found

Latest commit: 1af2f7b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Strift Strift changed the title Update instant-meilisearch to use meilisearch-js and ESM Update meilisearch-js and use ESM Jan 17, 2025
@flevi29
Copy link
Collaborator

flevi29 commented Jan 17, 2025

I'm really not sure what's going on.
image
Requiring works perfectly when I run it:
image
Import also works (other than the ESLint warning)
image

Seems like some of the failing tests here include similar errors.

@flevi29
Copy link
Collaborator

flevi29 commented Jan 17, 2025

Okay, I narrowed down the issue to this:

https://github.com/meilisearch/meilisearch-js/blob/489b58351c568e30a6c336d6e8bc9a603f17543d/vite.config.js#L42-L49

I still need to figure out why this causes the issue.

@Strift
Copy link
Collaborator Author

Strift commented Jan 17, 2025

@flevi29 I managed to get the playgrounds to work when using "type": "module", and get successful tests in local.
But I still need to figure out why they're not working in the CI though.

@flevi29
Copy link
Collaborator

flevi29 commented Jan 17, 2025

@Strift meilisearch/meilisearch-js#1829 should fix the remaining issue(s) 🤞
EDIT: I tried it locally, and it worked, although I didn't run all of the tests.

@Strift
Copy link
Collaborator Author

Strift commented Jan 20, 2025

Thanks for your help.

It's always the same tests failing. But I changed many dependencies in this branch, we might need to try from a clean branch.

@flevi29 here is what's happening when running the tests from a clean branch: #1367

@Strift
Copy link
Collaborator Author

Strift commented Jan 22, 2025

Superseded by #1373

@Strift Strift closed this Jan 22, 2025
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.

Update instant-meilisearch dependencies to use meilisearch-js v0.48
2 participants