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

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

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

hoelzro
Copy link
Contributor

@hoelzro hoelzro commented Oct 25, 2018

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.

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
Copy link
Contributor Author

hoelzro commented Oct 25, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

hoelzro commented Oct 29, 2018

@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
Copy link
Owner

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

@olivernn olivernn merged commit 8c6855f into olivernn:master Oct 30, 2018
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

Successfully merging this pull request may close these issues.

2 participants