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

Usage in meteor #1024

Closed
gwendall opened this issue May 22, 2016 · 18 comments · Fixed by #1037
Closed

Usage in meteor #1024

gwendall opened this issue May 22, 2016 · 18 comments · Fixed by #1037
Assignees

Comments

@gwendall
Copy link

gwendall commented May 22, 2016

I am having this error (Chrome 50.0.2661.102 / Meteor 1.3). Any idea ?

screen shot 2016-05-22 at 11 33 46 pm

@pixelastic
Copy link
Contributor

Hello,

I recently had another customer on support with a similar issue. In the end I feel like this is not coming from instantsearch (I checked all the source code, and we are never setting the connection, accept-encoding or content-length headers).

I think it has something to do with the way Meteor now handles npm modules, but I'm afraid that's the more I can help.

From what I understand, something, somewhere, on the Meteor side, is setting new headers to the queries the Algolia JavaScript client is doing. Because those headers are custom, and because the request is done to a domain different than the one where the page is hosted, then the CORS systems kicks in. It will then ask for a "preflight" OPTIONS call to our servers, that will fail because we are not configured to accept those custom headers.

Could you check in your network tabs the exact request that is sent to our API? I'm pretty sure the issue is not coming from our side but the way Meteor handles stuff.

@rayrutjes
Copy link
Member

@pixelastic actually the OPTIONS calls passes on our servers, see https://www.alittlemarket.com/.
@gwendall I think you simply have a CORS issue here, this answer might help: http://stackoverflow.com/a/19744754
Regarding the unsafe headers, you should let your browser handle these, otherwise it introduces security issues. I guess you'll have to find where they are set in your code or library and remove them according to http://stackoverflow.com/questions/7210507/ajax-post-error-refused-to-set-unsafe-header-connection.

Hope this helps!

@pixelastic
Copy link
Contributor

@rayrutjes I think that when an XMLHttpRequest is done with the withCredentials flag, it sends the Basic Auth informations along with the request. And in that case, the server must reply with an exact url in the Access-Control-Allow-Origin, but we always respond with a wildcard *, which fails.

So, I'm pretty sure another library is adding those headers to requests done to our servers.

@gwendall
Copy link
Author

Reproduction: https://github.com/gwendall/meteor-algolia-instantsearch-bug

@pixelastic
Copy link
Contributor

pixelastic commented May 23, 2016

Thanks for the repo. I managed to repro on my side, issue seems to have something to do with the request.js file that is weirdly called on every of our own XMLHttpRequest, like it whole object got extended...

I'm not experienced enough in Meteor to tackle that, I've asked a bit of help from @dustincoates, @SachaG and @acemtp

@SachaG
Copy link

SachaG commented May 23, 2016

Sorry, I don't know much about that aspect of Meteor. You should ask someone who knows more about it, like @gwendall for example. Oh wait…

@pixelastic
Copy link
Contributor

pixelastic commented May 23, 2016

In the Algolia JavaScript client, we have two paths. One for the front-end and one for the back-end (which is a valid separation in a non-Meteor context). They both share the same common core, but have a few special cases to handle in each environment. Like CORS for example. Back-end code does not care about CORS.

From what we understand, Meteor is trying to use the back-end build in the front-end (through browserify), while it should use the specific front-end build to do that. I have no idea how Meteor handles this kind of things...

@dustincoates
Copy link
Member

I spoke with @vvo about how instantsearch.js determines whether to use the front-end build or the back-end build. It's using browserify's package.json browser field spec as you can see here.

At a bit of a wall now. Current understanding is that Meteor is seeing the require on the server, not in the browser. Will try to look more into whether/how that can be overridden.

@tmeasday
Copy link

Meteor doesn't have any special support for the browser field. But you can certainly just require whichever file you like on the client side. So you could write something like

// Assuming that `client-require.js` was in the "browser" field of package.json
const instantsearch = require('instantsearch.js/client-require.js')

I can see that this issue comes from deeper in the dependency tree than that, so it's a little tricky, but that basic approach should lead to a solution I hope.

@vvo
Copy link
Contributor

vvo commented May 25, 2016

This is an issue on the meteor side unfortunately for now we cannot easily fix it: meteor/meteor#6890

Meteor does support the browser field but only when it's a single string, we have an object because we shim some weird dependency: https://github.com/algolia/algoliasearch-client-js/blob/870bee09e03908a3e7790f01d7d02278c496c0a8/package.json#L6

This might be fixable on the long run but meteor will have trouble since the browser "spec" is allowing objects so a lot of npm packages will fail: https://github.com/defunctzombie/package-browser-field-spec

@vvo
Copy link
Contributor

vvo commented May 25, 2016

We might be able to fix it if we get this merged: vercel/ms#40, thats the root cause of us having to add an object to our JS client.

@gwendall
Copy link
Author

gwendall commented May 25, 2016

What about using algoliasearch/lite in the instantsearch package ?

@vvo
Copy link
Contributor

vvo commented May 25, 2016

@gwendall yep that would work, I just need to put browse and browseFrom in the lite package because we have some tutorials using it.

@vvo vvo self-assigned this May 25, 2016
@vvo vvo changed the title CORS error Usage in meteor May 26, 2016
vvo pushed a commit that referenced this issue May 26, 2016
fixes #1024 by forcing to load /lite and avoid bad resolve of the
browser field.

This could break if people were using search.client to do things like
indexing. But we do document this as an API entry. We only have an
infinite scroll tutorial using the client passed to the helper. Thus I
had to make browse and browseFrom part of the lite build.
@jamiedumont
Copy link

Thanks for documenting the fix (a rarity I've found!). I've managed to replicate this fix locally (by temporarily amending where my shrink-wrap references an amended node_module; however, when do you expect this fix to make it to the published npm version?

I'm just wondering whether I freeze the dependancy to my custom version for now?

@vvo vvo closed this as completed in 219fa9f Jun 10, 2016
vvo pushed a commit that referenced this issue Jun 10, 2016
<a name="1.5.2"></a>
## [1.5.2](v1.5.1...v1.5.2) (2016-06-10)

### Bug Fixes

* **lite:** use lite algoliasearch build (js client) ([219fa9f](219fa9f)), closes [#1024](#1024)
* **poweredBy:** Let users define their own poweredBy template ([f1a96d8](f1a96d8))
@vvo
Copy link
Contributor

vvo commented Jun 10, 2016

Just did a release for that @jamiedumont, let me know how it goes! 1.5.2 on npm, jsDelivr in 30 minutes will be up

@jamiedumont
Copy link

Excellent, cheers! Perfect timing! Saw your other issue #920 that you're looking into some React Components. Would you want an extra set of hands to help with that? Finishing a massive release at the moment, but could spare a few hours a week after that to help out?

@vvo
Copy link
Contributor

vvo commented Jun 10, 2016

Would you want an extra set of hands to help with that?

We did not started that work yet, any help is welcomed. If you happen to create reusable React Components mimicking the instantsearch.js API then it would be awesome.

We will start building React components like instantsearch.js early july.

vvo pushed a commit that referenced this issue Jun 16, 2016
fixes #1024

Actually I had previously fixed #1024 by pointing to the lite build
(side effect).

But since https://github.com/algolia/algoliasearch-client-
js/issues/283 I had to make the client nodejs lite build point to the
nodejs normal build.

Then it would have tried to use the nodejs build in meteorjs on the
frontend.

Please meteorjs, fix this :)
vvo added a commit that referenced this issue Jun 16, 2016
fixes #1024

Actually I had previously fixed #1024 by pointing to the lite build
(side effect).

But since https://github.com/algolia/algoliasearch-client-
js/issues/283 I had to make the client nodejs lite build point to the
nodejs normal build.

Then it would have tried to use the nodejs build in meteorjs on the
frontend.

Please meteorjs, fix this :)
@vvo
Copy link
Contributor

vvo commented Jul 3, 2016

@yonidej This look like a different issue.

Can you please create a new issue and provide the code you are using (both html and JavaScript).

Thanks

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 a pull request may close this issue.

8 participants