-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix(nlu): no more weird debounce that override other intents #4317
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this, just add the return types so we don't miss autocomplete
train: () => Promise<void> | ||
cancelTraining: () => Promise<void> | ||
} | ||
export type NLUApi = ReturnType<typeof makeApi> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By doing this, you loose all the typing for return types .. we can move the return types in the impl istead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh that's why it was so dupplicated
sure then I'll do this
liteEditor?: boolean | ||
} | ||
|
||
export const IntentEditor: FC<Props> = props => { | ||
const [intent, setIntent] = useState<NLU.IntentDefinition>() | ||
|
||
const debouncedApiSaveIntent = useRef( | ||
_.debounce((newIntent: NLU.IntentDefinition) => props.api.createIntent(newIntent), 2500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the fix
useEffect(() => { | ||
// tslint:disable-next-line: no-floating-promises | ||
props.api.fetchIntent(props.intent).then(intent => { | ||
setIntent(intent) | ||
utils.inspect(intent) | ||
}) | ||
|
||
return () => debouncedApiSaveIntent.current.flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also the fix
Attempt at fixing botpress/v12#1185 + added few typings while I was there.
I'm not sure I actually solved the problem described in the issue, but I solved another one for sure:
when swicthing between intents (in the left pannel), if you switch before 2.5 seconds of deboucing, you override the current intent.
this is fixed now.