olivernn / lunr.js

A bit like Solr, but much smaller and not as bright
http://lunrjs.com
MIT License
8.89k stars 545 forks source link

Use Array.isArray to check if pipeline output is an array #381

Closed hoelzro closed 5 years ago

hoelzro commented 5 years ago

TiddlyWiki (https://tiddlywiki.com), when run on Node, loads all of its modules via Node's vm.runInContext. This means that for my full text search plugin (https://github.com/hoelzro/tw-full-text-search/), there is a pipeline function loaded in its own context, separate from the context lunr is loaded in. This mismatch of contexts causes result instanceof Array to always evaluate to false, which causes lunr to break in further ways down the line.

hoelzro commented 5 years ago

Here's a contrived example that reproduces the issue:

let lunr = require('./lunr.js');
let vm = require('vm');

let source = `
let noOpFilter = function noOpFilter(token) {
    console.log(token);
    return [token];
}

exports.noOpFilter = noOpFilter;
`;

let sandbox = {
    exports: {},
    console: console
};

vm.runInNewContext(source, sandbox);

let idx = lunr(function() {
    this.ref('id');
    this.field('body');
    this.pipeline.after(lunr.stopWordFilter, sandbox.exports.noOpFilter);

    this.add({
        id: 1,
        body: 'Colorless green ideas sleep furiosly'
    });
});

console.log(idx.search('green'));

Since the vm module is Node-specific, I couldn't think of a way to incorporate this test without breaking browser tests; if you have any ideas on how I could add a test for this, let me know!

olivernn commented 5 years ago

The change seems fine to me, support for Array.isArray is probably good enough (at least as good as Object.create which is used already).

I'm happy to make the change but would you mind explaining a bit more why this specific case requires it? I'm not familiar with either vm.runInContext nor TiddlyWiki's module loading.

hoelzro commented 5 years ago

@olivernn So what vm.runInContext does is run the specified code in its own little sandbox, so it can't affect the invoking environment. When code is compiled in this context, the prototypes of built-in objects, such as Array, different from context to context.

Let's say I create a function F in its own context, and then call it from another function G. F returns an Array named a, but that array's prototype points to the Array from F's context, which is different from the Array in G's context, so a instanceof Array will always return false within G.

olivernn commented 5 years ago

Ok, I understand, I'll merge and put together a release.