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

using MiniSearch.wildcard throws an error #42

Closed
roaminro opened this issue Jan 12, 2024 · 13 comments
Closed

using MiniSearch.wildcard throws an error #42

roaminro opened this issue Jan 12, 2024 · 13 comments

Comments

@roaminro
Copy link

Hi, @lucaong ! First, thanks for this amazing project, it's been a real pleasure working with this library!!!

I'm trying to use search(MiniSearch.wildcard) but for some reason doing so throws an error in the MiniSeach package. It looks like this check isn't working
https://github.com/lucaong/minisearch/blob/4fc49a43400b3dbce9f029f4c4a836ae56311928/src/MiniSearch.ts#L1489

And, so the next check is actually being triggered
https://github.com/lucaong/minisearch/blob/4fc49a43400b3dbce9f029f4c4a836ae56311928/src/MiniSearch.ts#L1493C27-L1493C27

and fails when trying to map the queries which don't exist on the wildcard Symbol
https://github.com/lucaong/minisearch/blob/4fc49a43400b3dbce9f029f4c4a836ae56311928/src/MiniSearch.ts#L1495
I wonder if the widlcard symbol isn't recognized because it's coming from another "import" of the MiniSearch package?

// the import that executes the query
import { useMiniSearch } from 'react-minisearch'
// the import where MiniSearch.wildcard comes from
import MiniSearch from 'minisearch'
@lucaong
Copy link
Owner

lucaong commented Jan 15, 2024

Hi @roaminro ,
thanks for the kind words 🙂

To be fair, I am not sure what is causing your issue. You might be right about the different imports, but I somehow cannot reproduce it. I added one test for wildcard search, which seems to pass though.

The test basically performs a wildcard search by simulating a click on this button, and seems to work fine. Are you doing anything different?

If you have time to share a small reproduction example, that would help me in figuring this out. For now, thanks for reporting this!

@roaminro
Copy link
Author

Thanks for looking into this @lucaong ! I tried to create a minimal reproduction example and you're right, it works fine (https://stackblitz.com/edit/react-starter-typescript-ifvmgd?file=App.tsx)

My setup involves Rollup, so I'm now wondering if something is happening during the bundling step which may cause that Symbol issue. It's going to be difficult to setup a minial example though, so I'm going to investigate a little more.

I'm going to close this issue for now, and will reopen it if I find what's the root cause ;)

@lucaong
Copy link
Owner

lucaong commented Jan 17, 2024

Thanks a lot @roaminro !
Do let me know if you figure out a fix, so I can document it here :)

@lyleunderwood
Copy link

Just ran into this issue, I suspect it is because the cjs MiniSearch files are being loaded by react-minisearch and the module MiniSearch files are being loaded by the bundler (webpack in my case), meaning there are literally two different definitions of the symbol occurring.

image

@lyleunderwood
Copy link

This is basically the same as #28

@lyleunderwood
Copy link

In reality the underlying problem here is that this package only delivers a UMD version. That seems like it should work fine, it's universal after all. But as far as bundlers are concerned, the UMD file is just a cjs module. And because of this, and because it uses require to load MiniSearch, the bundler gives it the cjs version of MiniSearch. In application code, we all want to use import, which our bundlers will happily give the ECMAScript module version of MiniSearch.

The most correct thing would be to do the thing that is fairly standard nowadays by delivering ECMAScript module and cjs versions (at least) and configuring the package.json to choose these correctly based on the bundler environment. MiniSearch itself seems to already be doing this correctly.

@lucaong
Copy link
Owner

lucaong commented Sep 19, 2024

Thanks for the investigation @lyleunderwood . I will work to release the ES module alongside the cjs package, instead of UMD, if that helps solving this issue.

lucaong added a commit that referenced this issue Sep 19, 2024
This should better serve different needs, and also solve
#42
@lucaong
Copy link
Owner

lucaong commented Sep 19, 2024

Relevant PR: #47

lucaong added a commit that referenced this issue Sep 19, 2024
This should better serve different needs, and also solve
#42
@lucaong
Copy link
Owner

lucaong commented Sep 19, 2024

Version v7.1.1 is released, with separate ESM, CJS, and UMD packages. @lyleunderwood or @roaminro would you be able to confirm that this fixes the issue?

@lyleunderwood
Copy link

Hey @lucaong thanks for the fast turnaround!

I can confirm that this resolves the issue for me, only the es files from MiniSearch are being bundled by webpack now, and I'm no longer getting the original error described above.

But I also have a regression! tsserver now complains that types are not being delivered correctly for react-minisearch:

Could not find a declaration file for module 'react-minisearch'. './node_modules/react-minisearch/dist/esm/react-minisearch.js' implicitly has an 'any' type.
There are types at './node_modules/react-minisearch/dist/react-minisearch.d.ts', but this result could not be resolved when respecting package.json "exports". The 'react-minisearch' library may need to update its package.json or typings. (tsserver 7016)

I'm using moduleResolution: bundler and the simple version is that typescript needs there to be a typing file that corresponds to each output module. MiniSearch seems to also be doing this correctly already.

https://arethetypeswrong.github.io/?p=react-minisearch%407.1.1

@lucaong
Copy link
Owner

lucaong commented Sep 19, 2024

@lyleunderwood thanks for reporting this issue with type declarations. I just released v7.1.2 that should fix it (as shown in https://arethetypeswrong.github.io/?p=react-minisearch%407.1.2).

@lyleunderwood
Copy link

Confirmed, types are now working in my environment, and I'm only loading MiniSearch once.

Thanks @lucaong !

@lucaong
Copy link
Owner

lucaong commented Sep 23, 2024

Awesome, thank you @lyleunderwood !

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

No branches or pull requests

3 participants