spellSuggest, promises, and express

[samartioli]

spellSuggest does not behave as expected when the callback is wrapped in a promise.

I created a repo to demonstrate the issue

[trofim24]

Very similar to nodejs/node#5691

[Wulf]

Hi @samartioli, thanks for reporting this and the great example you've setup!

I was able to reproduce this on linux! I tested this with the new nan branch, and the bug was no longer present (the promise resolved just fine). Thus, this will be fixed in the next major version ([email protected]).

In the next version, spellSuggest will no longer be present, only spellSuggestions is available. And this is renamed to suggest as per #37.

Below, I briefly outline some of the changes I made in order to test the nan branch here:

1. Switch to the suggest function, instead of spellSuggest. origWord isn't passed in currently, so I use the utterance parameter instead later on instead.
2. an array is always sent, so I had to check for suggestion.length > 0, instead of just if(suggestion).
3. resolve to the first element in the array instead of the array.

function spellSuggestPromise(utterance) {
    return new Promise(function(resolve, reject) {
        dict.suggest(utterance, function(err, correct, suggestion, origWord) { // 1
            console.log(err, correct, suggestion, origWord);
            if (err) {
                // return setImmediate(reject, error);
                return reject(error);
            }
            if (suggestion.length > 0) { // 2
                return resolve(suggestion[0]); // 3
                // return setImmediate(resolve, suggestion);
            }
            return resolve(utterance);
            // return setImmediate(resolve, origWord);
        });
    });
}

Again, thanks for your patience and time setting up nodehun-promise-example. :)

[ashutoshrishi]

I am currently experiencing this with the master branch, and resorting to using setImmediate for now. Should I be using the nan now ?

[Wulf]

The NAN implementation isn't complete (see #37). I'll look into this in the master branch.

[loretoparisi]

@Wulf any update on this?
In the meanwhile my my wrapper I had to use the setImmediate as well like this

Hunspell.prototype.isCorrectP = function(word) {
        var self = this;
        return new Promise(function(resolve, reject) {
            self.dictionary.isCorrect(word, function(err, correct, origWord){
                // because "color" is a defined word in the US English dictionary
	            // the output will be: null, true, "color"
                if(err) reject(err)
                else return setImmediate(resolve, correct);
            });
        });
    };
Hunspell.prototype.spellSuggestionsP = function(word) {
        var self = this;
        return new Promise(function(resolve, reject) {
            self.dictionary.spellSuggestions(word, function(err, correct, suggestions, origWord){
                // because "calor" is not a defined word in the US English dictionary
	            // the output will be: null, false, [ 'carol','valor','color','cal or','cal-or','caloric','calorie'], 'calor'
                if(err) reject(err)
                else return setImmediate(resolve, suggestions);
            });
        });
    };
Hunspell.prototype.stemP = function(word) {
        var self = this;
        return new Promise(function(resolve, reject) {
            self.dictionary.stem(word, function(err, stems){
                // the output will be: null, [telling, tell]
                if(err) reject(err)
                else return setImmediate(resolve, stems);
            });
        });
    };

[Wulf]

@samartioli @ashutoshrishi @loretoparisi thanks for your patience everyone.

Async methods return Promises directly now in the n-api branch.
They will be featured in 3.0.0 which should be released in the following few days.

If you're still around, please let me know if the new version fixes the issue:

npm i -S wulf/nodehun#n-api

The API has changed a bit, but feel free to reference the type declaration file.

[Wulf]

I got a chance to verify that this doesn't occur with Nodehun v3 on node version 10, 11 and 12.

I changed the definition of the spellSuggestPromise function in samartioli's repository to the following:

function spellSuggestPromise(utterance) {
    return dict.suggest(utterance)
}

Thanks again for reporting this in a detailed manner and making it easy for us to reproduce the error. I'm going to close this issue, but feel free to reopen it or continue the discussion.