search

hoelzro / lunr-mutable-indexes

Mutable indexes for lunr.js 2.1.x
MIT License
20 stars 4 forks source link

Pull for Sugar #1

Closed k00p closed 6 years ago

k00p commented 6 years ago

Went ahead and got this project all set up so that you can just pull from npm and go. Things were rearranged quite a bit by stashing the MutableBuilder and MutableIndex behind a 'lunrMutable' object to implement the 'syntactic sugar'. If there is a cleaner or better approach, I would be OK with it.

To get things working correctly from a npm install, the lunr-mutable.js had to be included in the repo, so I removed it from .gitignore. To compensate, I added a make clean target to the Makefile. The Make stuff seems clunky, but this is the first published npm module that I have contributed to, so maybe that's just how it's done?

Also, the README is updated with the latest instructions and a reference back to the lunr pull request. It would be nice if @olivernn decides to bring the mutable changes back into lunr, but having this module to update against will be suitable in the interim.

The next target IMHO is definitely to go after the index rebuild problem, but this will work for now. Thanks again @hoelzro for bringing this to life!

hoelzro commented 6 years ago

@k00p It's so funny - I was just thinking about this on Wednesday morning! Thanks so much for contributing; I'll take a look today or tomorrow.

hoelzro commented 6 years ago

Looks good to me! I'd prefer not to have build outputs in source control, but I think I might be able to remove that using NPM scripts. I'm going to merge this, but one quick question: I noticed package-lock.json is both in .gitignore and checked in to the repository. Should it be ignored, or should it be checked in?

hoelzro commented 6 years ago

Also, thanks again for contributing!

hoelzro commented 6 years ago

Aaaand I just released version 0.2.0 to NPM with the changes!

k00p commented 6 years ago

Nice!! That package-lock.json appears to be a npm artifact that "is intended to be committed into source repositories". So perhaps we should remove it from .gitignore and leave it in the repo. Sorry about that! I just updated to a version of npm that generates it so should have done some more research.

Hope to contribute more as we progress!

hoelzro commented 6 years ago

@k00p Is that just for applications, or for libraries too? I looked at some other NPM libraries and I didn't see package-lock.json in their repos, so I removed it. We can always restore it if needed, though.

k00p commented 6 years ago

There is a ton of discussion on package-lock.json in this Stack Exchange question and whether to include it. It seems like the majority end up removing it as it appears to cause more issues than it solves.