fletcherist / yandex-dialogs-sdk

☂️Build your skill for Yandex.Alice with ease. (nodejs/typescript)
MIT License
122 stars 16 forks source link

Об обязательности возвращения результата из команды scene #60

Closed popstas closed 6 years ago

popstas commented 6 years ago

Второй раз натыкаюсь на проблему: если из команды сцены не вернуть результат, навык сломается.

В 1.4.7 ctx.reply() перестал возвращать результат, из-за этого все мои сцены сломались (вся одна сцена), т.к. они заканчивались на return ctx.reply(), этот пример из README.md тоже должен перестать работать:

const inBar = new Scene('in-the-bar')
inBar.enter('Алиса, пойдём в бар!', ctx => ctx.reply('Пойдём.'))
inBar.command('ты сейчас в баре?', ctx => ctx.reply('Да!'))
inBar.leave('Пошли отсюда', ctx => ctx.reply('Уже ухожу.'))

Если команда сцены не вернет положительный (ненулевой) результат, выполнение продолжится, commands.search() найдет и исполнит следующую команду, из-за этого.

В то же время, если не вернуть результат из обычной команды, ничего страшного не произойдет, в тех же examples и реальных использованиях это есть.

Мои предложения:

alexander-karpov commented 6 years ago

Как насчет того, чтобы вместо вызова ctx.reply() использовать возвращаемое значение?

inBar.enter('Алиса, пойдём в бар!', () => 'Пойдём.')
inBar.command('Ты сейчас в баре?', () => 'Да!')

Сейчас ведь ctx.reply обязательно должен быть вызван в обработчике команды и (скорее всего) только один раз - это хорошо подходит для return. К тому же возвращаемое значение сейчас не используется.

inBar.enter('Алиса, пойдём в бар!', async ctx => {
    ctx.reply('Пойдём.')
    await ...
    ctx.reply('Но не сегодня.')
})

@popstas ctx.reply()ничего полезного не возвращал, лучше первый вариант.

popstas commented 6 years ago

@alexander-karpovl, согласен, возможность возвращать строку в команде была бы удобна, только в предложенном тобой способе внутри sdk наверное будет сложно определить, надо ли использовать возвращаемое значение как ответ, или функция уже ответила и возвращает true или другой малозначащий результат.

Я бы сделал так:

inBar.command('Ты сейчас в баре?', 'Да!')

Это было бы аналогично простому матчеру по строке и запись получается совсем короткая.

alexander-karpov commented 6 years ago

@popstas Да, сложно. Есть вариант оставить только return и сделать ctx.reply deprecated, либо добавить проверку, что если команда вызывает ctx.reply, она не должна ничего возвращать.

Я бы сделал так:

Все равно нужно место для какой-то логики команды. Как в этом примере

alice.command('Забронируй встречу в ${where} на ${when}', async ctx => {
    const { where, when } = ctx.body;
    await scheduleMeeting(where, when);

    return `Готово. Встреча состоится в ${where}. Вам напомнить?`;
});
popstas commented 6 years ago

@alexander-karpov, мне кажется, что через явный вызов функции отвечать правильнее, т.к. все равно будут места, где нельзя просто вернуть строку. В моем навыке ctx.reply() используется мало, т.к. почти всегда с ответом возвращаются кнопки.

Матчеры тоже у меня сложные и почти не используется просто одна строка как матчер. Эта фича скорее будет полезна для тех, кто только знакомится с фреймворком, после этого все равно программист должен прийти к вариативности запросов и ответов, кнопкам, матчерам и т.д.

alexander-karpov commented 6 years ago

По повожу кнопок: можно сделать чтобы return в команде принимал тот же формат, что и reply:


const { button } = Alice;

alice.welcome(ctx => {
    return {
        text: 'Привет! Я загадала число от 1 до 100. Сможешь отгадать его?',
        buttons: [button('Давай попробуем!'), button('Не хочу')]
    }
});
popstas commented 6 years ago

Есть еще другие варианты ответа, тогда надо и для них продумать ответ:

Либо написать в доке, что для сложных случаев функции, для простых - возвращать объект в определенном формате.

alexander-karpov commented 6 years ago

Да, не клеится что-то. Последний вариант - возвращать результат ReplyBuilder :)

alice.welcome(() =>
    ReplyBuilder.tts(
        'Привет! Я загадала число от 1 до 100. Сможете отгадать его?',
        ReplyBuilder.button('Давай попробуем!'),
        ReplyBuilder.button('Не хочу')
    )
);
fletcherist commented 6 years ago

Привет! Мне очень понравилось предложение @alexander-karpov сделать возможность отвечать без ctx.reply. Это очень круто, давайте так и сделаем. А вот только зачем депрекейтить ctx.reply? Пускай разработчики используют, что хотят. Мне вот, например, нравится явный вызов метода reply (потому что так принято во многих фреймворках, и разработчику легче адаптироваться), но, с другой стороны, мне кажется, что обычный return чище и логичнее.

inBar.enter('Алиса, пойдём в бар!', () => 'Пойдём.')
inBar.command('Ты сейчас в баре?', () => 'Да!')

@popstas, крутой вброс, но он ограничивает ветвление сценария. Здесь невозможно ничего проверить и ответить «нет» вместо «да». Вариант возвращать из функции сразу ответ крутой, посколько в нём можно что-то проверять и менять сценарий на лету.

inBar.command('Ты сейчас в баре?', 'Да!') // bad
inBar.command('Ты сейчас в баре?', () => 'Да!') // good
popstas commented 6 years ago

Я понимаю, что проверить ничего нельзя, и в этом такое поведение полностью соответствует поведению матчера-строки ) Вариант bad короче, а good гибче, но почему тогда не:

inBar.command(() => 'Ты сейчас в баре?', () => 'Да!') // good

Меня вот эта неконсистентность только смутила. А вообще да, если разрешить возвращать строку из любой функции, это будет гибче. С третьей стороны можно и оба варианта реализовать, в виде функции и в виде строки )

alexander-karpov commented 6 years ago

@fletcherist А что ты думаешь по поводу как вернуть tts, кнопки и картинки?

fletcherist commented 6 years ago

@alexander-karpov дак так же, смотрите: https://github.com/fletcherist/yandex-dialogs-sdk/blob/master/src/tests/reply.spec.ts#L35

https://github.com/fletcherist/yandex-dialogs-sdk/blob/master/src/card.ts#L4

const { reply, replyWithImage } = Alice
inBar.command('Ты сейчас в баре?', () => {
   return reply({tts: 'да', text: 'да'})) 
}
inBar.any(() => {
   return replyWithImage(IMAGE_ID)
   // same as ctx.replyWithImage(IMAGE_ID)
}
fletcherist commented 6 years ago

@popstas дак нет никаких проблем так писать. Это уж как разработчик захочет:

inBar.command(() => 'Ты сейчас в баре?', () => 'Да!') // good
alexander-karpov commented 6 years ago

Если никто не против, возьмусь.

fletcherist commented 6 years ago

@alexander-karpov we released 2.0 yesterday. Now it supports that syntax:

inBar.command(() => 'Ты сейчас в баре?', () => Reply.text('Да!'))
inBar.command(() => 'Ты сейчас в баре?', () => ({text: 'Да!'}))

Do we still need to simply return string from handler? I found it to be not very obvious