manastech / middleman-search

LunrJS-based search for Middleman
MIT License
58 stars 31 forks source link

Please update lunr.js #25

Open sn3p opened 7 years ago

sn3p commented 7 years ago

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

Thanks in advance!

matiasgarciaisaia commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

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

sn3p commented 7 years ago

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.

olivernn commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 6 years ago

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

macandcheese commented 6 years ago

Any chance of merging this in?