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

tutorialStore onDocumentChanged unexpected behaviour #433

Open
so0k opened this issue Jan 11, 2025 · 1 comment
Open

tutorialStore onDocumentChanged unexpected behaviour #433

so0k opened this issue Jan 11, 2025 · 1 comment
Labels
bug Something isn't working pr-welcome Accepting pull requests for this change

Comments

@so0k
Copy link

so0k commented Jan 11, 2025

Describe the bug

using the tutorialStore.onDocumentChanged triggers callback immediately because the nanostores subscribe callback seems to work that way.

For example:

    const terminal = tutorialStore.terminalConfig.value!.panels[1];
    const filePath = '/foo';
    tutorialStore.onDocumentChanged(filePath, (document) => {
       terminal.write(`${document.filePath} changed\n`);
    });

Expected comparison between old and new document to confirm changes happened

    const terminal = tutorialStore.terminalConfig.value!.panels[1];
    const filePath = '/foo';
    const stopListening = tutorialStore.documents.listen((newDocuments, oldDocuments) => {
      if (!newDocuments[filePath] || !oldDocuments || !oldDocuments[filePath]) {
        return;
      }
      const oldDocument = oldDocuments[filePath];
      const newDocument = newDocuments[filePath];
      if (oldDocument.value !== newDocument.value) {
        queueMicrotask(() => {
          terminal.write(`${newDocument.filePath} changed\n`);
          stopListening();
        });
      }
    });

Link to a StackBlitz project which shows the error

https://stackblitz.com/~/github.com/so0k/tutorialkit-terminal-writer

Steps to reproduce

  1. click "Test" button
  2. Notice terminal shows "Document changed" when it did not

Expected behavior

  1. click "Test" Button
  2. Modify foo (i.e. use TutorialKit API to update foo)
  3. only then, notice callback is triggered

Screenshots

No response

Platform

  • TutorialKit version: 1.3.1
  • OS: local Linux, Stackblitz
  • Browser: Chrome
  • Version: 131.0.6778.204 (Official Build) (64-bit)

Additional context

  1. Consider using listen if we don't want immediate callback?
  2. Compare newValue(s) to oldValue(s) before triggering?
@AriPerkkio
Copy link
Member

because the nanostores subscribe callback seems to work that way.

This seems to be the root cause indeed:

store.subscribe(cb) in contrast with store.listen(cb) also call listeners immediately during the subscription.

https://github.com/nanostores/nanostores?tab=readme-ov-file#atoms

onDocumentChanged(filePath: string, callback: (document: Readonly<EditorDocument>) => void) {
const unsubscribeFromCurrentDocument = this.currentDocument.subscribe((document) => {
if (document?.filePath === filePath) {
callback(document);
}
});
const unsubscribeFromDocuments = this.documents.subscribe((documents) => {
const document = documents[filePath];
/**
* We grab the document from the store, but only call the callback if it is not loading anymore which means
* the content is loaded.
*/
if (document && !document.loading) {
/**
* Call this in a `queueMicrotask` because the subscribe callback is called synchronoulsy,
* which causes the `unsubscribeFromDocuments` to not exist yet.
*/
queueMicrotask(() => {
callback(document);
unsubscribeFromDocuments();
});
}
});

The onDocumentChanged is not used internally by TutorialKit, it's only available via the experimental API. Fixing this should be quite risk-free.

@AriPerkkio AriPerkkio added bug Something isn't working pr-welcome Accepting pull requests for this change labels Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pr-welcome Accepting pull requests for this change
Projects
None yet
Development

No branches or pull requests

2 participants