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

Please update lunr.js #25

Open
sn3p opened this issue Apr 14, 2017 · 11 comments · May be fixed by #29
Open

Please update lunr.js #25

sn3p opened this issue Apr 14, 2017 · 11 comments · May be fixed by #29

Comments

@sn3p
Copy link

sn3p commented Apr 14, 2017

The gem currently includes lunr.js v0.7.0, but the latest version is v2.0.1.

Thanks in advance!

@matiasgarciaisaia
Copy link
Member

Wow - that escalated quickly. Tomorrow will be one year since we updated to 0.7.0 - and they now are at 2.0.1! :)

A PR would be really welcome for this. If not, I'll try to do this whenever I have some time - I can't promise that'd be soon, though.

Thanks for the heads up!

@sn3p
Copy link
Author

sn3p commented Apr 20, 2017

@matiasgarciaisaia sure, glad to help!

Just tried updating to lunr.js v2.0.2 but I'm getting this error for every resource:

Error processing resource for index: index.html
undefined method `add' for #<V8::Object:0x007f8ab3c37f40>
 /Users/snap/.rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/gems/therubyracer-0.12.3/lib/v8/object.rb:69:in `method_missing'
 /Users/snap/Git/middleman-search/lib/middleman-search/search-index-resource.rb:98:in `block (2 levels) in build_index'

For some reason index.add is not a function anymore:
index.respond_to?(':add') in lunr.js v2.x.x and up returns false.

Any clue why that could be?

@matiasgarciaisaia
Copy link
Member

A V8::Object is the Ruby representation of the Javascript objects that TheRubyRacer manages.

If you get an undefined method error there - while upgrading a JS library version - that means the library's API changed. That would explain the major version being changed - Semantic Versioning says you should upgrade the major version whenever you make an API-breaking change in your code.

There's an Upgrading guide at lunrjs.com that specifies that the API changed about indexing, so you should take a look at that for upgrading middleman-search.

The upgrade could be easy to do, or maybe not - I'm not sure how the lunr.js changes will impact on the extension.

Give it a shot and, if you get to a dead end, ping me back next week 👍

@matiasgarciaisaia
Copy link
Member

For a starter, lunr.js doesn't provide a minified lunr.min.js on it's repo since v2.0 (since olivernn/lunr.js@2a57c53, actually). So we should handle minifying it somehow.

Then we have to adapt our code so it's compatible with the new API.

@sn3p
Copy link
Author

sn3p commented May 2, 2017

Then we have to adapt our code so it's compatible with the new API.

Thanks for the pointers. Unfortunately I've had very little time to get this going, but I've made some small progress. After implementing the new API changes all seemed fine, and the specified pages were indexed. But after refreshing search.json in the browser it was appeared empty.
If I find the time I'll try to pick this up again, in the meanwhile I can create a WIP PR of my progress if you like. Maybe you have some insights on what might be the problem.

For a starter, lunr.js doesn't provide a minified lunr.min.js on it's repo since v2.0 (since olivernn/lunr.js@2a57c53, actually). So we should handle minifying it somehow.

Yea I noticed this as well, so I minified it manually for now.

Also, in middleman-search the language files (i18n) are not minified. But lunr-languages now offers minified files, so I suggest including those (or both?). They forgot to minify the Japanese files, which I brought to their attention in a an issue.

@sn3p sn3p linked a pull request May 16, 2017 that will close this issue
3 tasks
@olivernn
Copy link

olivernn commented Jun 5, 2017

Hey, if you have any questions about the changes or run into any problems with Lunr please let me know, I'll try and help if I can.

@gerwitz
Copy link

gerwitz commented Jun 23, 2017

I also have problems with a second generation of search.json during a single middleman search session. This using the current release (and also the current Middleman), so the problem here may not be yours, @sn3p.

@sn3p
Copy link
Author

sn3p commented Jun 23, 2017

@gerwitz thanks for letting me know. I've started a PR to update lunr.js and its new implementation but haven't found the time to get it working properly. Feel free to dig in!

@gerwitz
Copy link

gerwitz commented Jun 27, 2017

FWIW, I'm using @sn3p's update branch and the only change I'm seeing is a much lighter search.json. (At https://github.com/gerwitz/hans.gerwitz.com/ )

@eemi
Copy link

eemi commented Sep 11, 2017

I can confirm that this is working just fine, in our case the index file went from 650kb to 770kb.

@macandcheese
Copy link

Any chance of merging this in?

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