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

Make lunr.js compatible with IE8 #32

Closed
wyuenho opened this issue May 19, 2013 · 10 comments
Closed

Make lunr.js compatible with IE8 #32

wyuenho opened this issue May 19, 2013 · 10 comments

Comments

@wyuenho
Copy link

wyuenho commented May 19, 2013

Right now the reference to undeclared vars console gives script errors in IE8. The use of ES5 method forEach makes lunr.js incompatible with IE8 too. lunr.js is perfectly capable of running in IE8 with reasonable speed, it'd be nice if these tiny issues could be fixed. If not, at least put up a notice somewhere that says which browser version is supported.

@Venemo
Copy link

Venemo commented May 19, 2013

I think the docs clearly state that you need to use an ES5 shim in older browsers.

@wyuenho
Copy link
Author

wyuenho commented May 19, 2013

Yep u are right, I was in a hurry and forgot to double check.

But then again, why? ES5 doesn't shim console and all lunr.js uses is forEach. Is it really that hard to switch to plain-old for loops? It's even faster than using callbacks with forEach.

@olivernn
Copy link
Owner

lunr uses more than just Array.prototype.forEach, there are also reduce, map and some calls in the code.

It would be possible to switch to using for loops in these cases but I think using the more modern methods makes the code simpler, easier to read and maintain.

In some more performance sensitive areas of the code lunr does use for loops, but this is a performance optimisation.

Whilst these methods could be shimmed in lunr, since I already maintain augment.js I don't really want to duplicate those efforts, also it means that you can use whatever shim you want.

I'll take a closer look at the console error, I thought the check around the call to console.warn would prevent these errors but clearly this isn't working.

I'll also include augment.js in the tests though since they should all pass regardless of what browser you are using.

@olivernn
Copy link
Owner

This has been fixed in version 0.3.3

@andreif
Copy link

andreif commented Aug 21, 2013

@olivernn we are still having errors in IE8 with lunr v0.4.1. Could you point me to a commit fixing the issue?

@olivernn
Copy link
Owner

@andreif what errors are you seeing? Could you put together a simple jsfiddle with a reduced test case that reproduces the errors?

Also are you including an ES5 shim, either augment.js, es5-shim or similar?

@andreif
Copy link

andreif commented Aug 21, 2013

@olivernn no, I don't include an ES5 shim, but now I see that I should. Which of them do you use?

Edit: oh, that was a stupid question :)

@olivernn
Copy link
Owner

Well I'm slightly biased here since I maintain augment.js so thats the one I use, however es5-shim is also very good. One (potential) advantage of augment.js is that you can pick and choose what shims you actually include, this may or may not be an issue for you.

Either one would be a fine choice though and will allow you to take advantage of the features of ES5 in your code.

@andreif
Copy link

andreif commented Aug 21, 2013

Oh, now I regret not reading your previous comments completely. I wonder what stopped me from doing that...

@olivernn
Copy link
Owner

That's ok, let me know if you have any other problems with any version of IE you use lunr.js in.

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

4 participants