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

Why does instantsearch require React? #2388

Closed
millansingh opened this issue Sep 26, 2017 · 19 comments
Closed

Why does instantsearch require React? #2388

millansingh opened this issue Sep 26, 2017 · 19 comments

Comments

@millansingh
Copy link

I've installed instantsearch via npm, and it installed fine. However, when I include it in a script and compile the script (with Gulp/Browserify), I get console errors saying that it can't find the 'react' module in various places (like 50 places).

I asked my boss, and he told me to just install react, but I have to wonder why this would use react in the first place, especially when there's a whole separate library specifically for react.

Can someone provide some clarification here on why react is a dependency?

@Haroenv
Copy link
Contributor

Haroenv commented Sep 26, 2017

preact is used internally (with an alias to react), but you should normally not be aware of this as an end-user. Could you share your configuration that causes these error messages so that you're correctly using the production (preact) version?

Thanks!

@iam4x
Copy link
Contributor

iam4x commented Sep 26, 2017

This is related to #2279

Until we come up with a real fix, you can try to use https://github.com/tleunen/babel-plugin-module-resolver and resolve react to preact-compat like @thangngoc89 said here #2279 (comment)

@millansingh
Copy link
Author

Haroenv, it's a default installation of instantsearch. The site does not use React or Preact for anything.

@Haroenv
Copy link
Contributor

Haroenv commented Sep 28, 2017

I assumed that @millansingh, but this is likely an issue coming up because of a particular setup of build tools that wasn't anticipated before, so for us to find in which use cases this happens exactly would be very useful in replicating and fixing it. Something helpful here is a list of versions used, or even better a reproducible project

@millansingh
Copy link
Author

I can't provide something reproducible, since it's a work project. However, I can tell you that we're building front-end files with Gulp and it's within a Laravel instance, using some of Laravel's Elixir Gulp stuff.

@iam4x
Copy link
Contributor

iam4x commented Sep 28, 2017

Another workaround would be to use the UMD build in the meantime @millansingh. How do you import InstantSearch.js into your code?

@millansingh
Copy link
Author

It's imported by a simple require statement, and downloaded as an npm package.

bobylito pushed a commit that referenced this issue Oct 2, 2017
* test(babel): alias preact to react for testing purpose
* feat(preact): replace `react` and `react-dom` occurences
* fix(SLider.Pit): use key to compute pitValue when available
* test(widgets): rewire preact now instead of ReactDOM

Fix #2279 #2388

I replace `React` and `ReactDOM` by `preact-compat` and fixed the tests which were failing because of the update.

I've also replaced Rheostat `React` with `preact-compat` in the fork.
@hallcyon11
Copy link

this is still happening, i have to install react and react-dom for it to work

Error: Cannot find module 'react' from '/Users/asdf/node_modules/instantsearch.js/dist-es5-module/src/widgets/menu-select'
    at /Users/asdf/node_modules/browser-resolve/node_modules/resolve/lib/async.js:46:17
    at process (/Users/asdf/node_modules/browser-resolve/node_modules/resolve/lib/async.js:173:43)
    at ondir (/Users/asdf/node_modules/browser-resolve/node_modules/resolve/lib/async.js:188:17)
    at load (/Users/asdf/node_modules/browser-resolve/node_modules/resolve/lib/async.js:69:43)
    at onex (/Users/asdf/node_modules/browser-resolve/node_modules/resolve/lib/async.js:92:31)
    at /Users/asdf/node_modules/browser-resolve/node_modules/resolve/lib/async.js:22:47
    at FSReqWrap.oncomplete (fs.js:152:21)

@Haroenv
Copy link
Contributor

Haroenv commented Oct 12, 2017

Can you make a repository where this happens @hallcyon11? This should be fixed in v2.2.0

@hallcyon11
Copy link

"instantsearch.js/dist-es5-module/src/widgets/menu-select/menu-select.js" requires react and react-dom and "instantsearch.js/dist-es5-module/src/components/MenuSelect.js" requires react. I changed them to preact-compat and it compiled...

@Haroenv
Copy link
Contributor

Haroenv commented Oct 12, 2017

Oh, you're right, #2460 was merged but not yet released. Can you try v2.2.1-beta.0?

@hallcyon11
Copy link

idk how to do that, i'll just stick with what i have until v2.2.1 is released, thank you for the help.

@nilsi
Copy link

nilsi commented Oct 15, 2017

I tried v2.2.1-beta.0 but it diden't help. I still having some 'react' dependencies like the screenshot shows:

screen shot 2017-10-15 at 15 38 22

@hallcyon11
Copy link

just do what i did

@nilsi
Copy link

nilsi commented Oct 15, 2017

If I change them manually it compiles but Im getting another problem:
Uncaught Error: Cannot find module 'rheostat'

I saw after doing an npm install that:
npm WARN rheostat@2.1.1 requires a peer of react@>=0.13.x but none was installed.

Its related to the range slider. Is there any plans of changing this to preact as well?

@iam4x
Copy link
Contributor

iam4x commented Oct 16, 2017

Hello, we have just release v2.2.1 which includes the fix 👍

@iam4x iam4x closed this as completed Oct 16, 2017
@Haroenv
Copy link
Contributor

Haroenv commented Oct 19, 2017

Weird! I thought #2460 fixed that first problem, was it reverted @iam4x ?

Thanks for your investigation @nilsi

@Haroenv
Copy link
Contributor

Haroenv commented Jun 1, 2018

I think that this is fixed now. Please open a new issue if you have issues regarding wrong aliasing of React

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants