OvidijusParsiunas / deep-chat

Fully customizable AI chatbot component for your website
https://deepchat.dev
MIT License
1.27k stars 176 forks source link

next build & next start does not work, it always is demo version. #71

Closed Pai-Po closed 6 months ago

Pai-Po commented 6 months ago

Hello, I tried adding the "request" attribute in , but I found that it works fine when executing "next dev" in next.js. However, when I execute "next build & next start", it always considers it as a demo, even if I didn't add the "demo" attribute. I tried debugging and found that the issue occurs in onRender. When onRender is called for the first time, e.request and some other attributes are empty, which leads to the default execution of the demo process. Can you help me with this? image image image image

OvidijusParsiunas commented 6 months ago

Hi @Pai-Po. Thankyou for raising the issue. I have been trying to reproduce your problem using the nextjs server example and it seems to work fine for me.

Here is the code that I used in the index.tsx file:

import styles from '../styles/Index.module.css';
import dynamic from 'next/dynamic';

export default function IndexPage() {
  const DeepChat = dynamic(() => import('deep-chat-react').then((mod) => mod.DeepChat), {
    ssr: false,
  });

  return (
    <>
      <main className={styles.main}>
        <div className={styles.components}>
          <DeepChat request={{url: '/api/custom/chat-stream'}} stream={true} />
        </div>
      </main>
    </>
  );
}

Then I ran npm run dev, npm run build and npm run start and it worked for me.

Could you perhaps try to replicate the issue by forking the nextjs live example so I could see it live.

Thanks!

Pai-Po commented 6 months ago

I tried deploying on codesandbox, but couldn't reproduce it; however, I found the cause of the issue. When DeepChat is initializing, onNewMessage is instanced,

  @Property('function')
  onNewMessage: OnNewMessage = () => {};

it will trigger the setter

     const setter = function (this: InternalHTML, newVal: string) {
      value = newVal;
      if (attributeName) {
        // if the lower case version - assign the value to the actual property
        (this as unknown as GenericObject)[propertyKey] = newVal;
      } else {
        RenderControl.attemptRender(this);
      }
    };
    Object.defineProperty(this, attributeName || propertyKey, {
      get: getter,
      set: setter,
    });
  }

afterwards, the setter will attempt to call attemptRender, which will further result in the execution of onRender

 export class RenderControl {
  private static waitForPropertiesToBeUpdatedBeforeRender(deepChat: InternalHTML) {
    deepChat._propUpdated_ = false;
    setTimeout(() => {
      if (!deepChat._propUpdated_) {
        deepChat._waitingToRender_ = false;
        deepChat.onRender();
      } else {
        RenderControl.waitForPropertiesToBeUpdatedBeforeRender(deepChat);
      }
    });
  }

  public static attemptRender(deepChat: InternalHTML) {
    deepChat._propUpdated_ = true;
    if (!deepChat._waitingToRender_) {
      deepChat._waitingToRender_ = true;
      RenderControl.waitForPropertiesToBeUpdatedBeforeRender(deepChat);
    }
  }
}

the factory function will be executed in onRender

  override onRender() {
    this._activeService ??= ServiceIOFactory.create(this); // not re-connecting if re-rerending

however, at this moment, the values of the request and some other attributes are empty, which will result in an execution of BaseServiceIO

     if (request) {
      return new BaseServiceIO(deepChat);
    }
    // when directConnection and request are not defined, we default to demo
    return new BaseServiceIO(deepChat, undefined, demo || true);
  }

finally, when BaseServiceIO is called, demo is set to True. But I'm not sure why this process would be triggered

1 image 2 image 3 image 4 image 5 image 6 image 7 image

OvidijusParsiunas commented 6 months ago

That is interesting.

I am quite confused why the example NextJs project and the live NextJs project work with the same code, can you tell me more about what is the difference in those projects compared to your project? Are you perhaps using a different version of NextJs? Any information would be useful. Thankyou!

OvidijusParsiunas commented 6 months ago

By default the - onNewMessage is triggering an attempt to render but it should theoretically not call the onRender function. I made a small change in the code and deployed it to deep-chat-dev and deep-chat-react-dev packages version 9.0.102 which will stop this onNewMessage from attempting to re-render the component. These packages work exactly the same as core ones, except their names are different. Let me know if this helps you.

Pai-Po commented 6 months ago

I do further analyzed it and found that the problem lies in RenderControl. When the setter of onNewMessage reaches RenderControl, the code uses _propUpdate to determine if all the properties have been fully updated. Only when the update is complete, onRender will be executed further. There is an implicit race condition here, which is that an attempRender must occur between setting the callback with setTimeout and the actual execution process to ensure _propUpdated = true. However, this time window cannot guarantee that it will always be succeed in the competition since the window is too small, which is why my error occurred. I tried to make the following code modifications. Please help me evaluate them. I still need to further check the initialization of class member variables, constructors, and the decorators in TypeScript classes.

export class RenderControl {
  private static waitForPropertiesToBeUpdatedBeforeRender(deepChat: InternalHTML, depth = 0) {
    deepChat._propUpdated_ = false;
    setTimeout(() => {
      if (!deepChat._propUpdated_) {
        deepChat._waitingToRender_ = false;
        deepChat.onRender();
      } else if (depth < 10) {                                  // Limit recursion depth to prevent stack overflow
        RenderControl.waitForPropertiesToBeUpdatedBeforeRender(deepChat, depth + 1);
      }
    }, 100);                                                            // Wait for 100ms before checking if properties have been updated
  }

  public static attemptRender(deepChat: InternalHTML) {
    deepChat._propUpdated_ = true;
    if (!deepChat._waitingToRender_) {
      deepChat._waitingToRender_ = true;
      RenderControl.waitForPropertiesToBeUpdatedBeforeRender(deepChat);
    }
  }
}
Pai-Po commented 6 months ago

That is interesting.

I am quite confused why the example NextJs project and the live NextJs project work with the same code, can you tell me more about what is the difference in those projects compared to your project? Are you perhaps using a different version of NextJs? Any information would be useful. Thankyou!

I can only confirm that the code between these two is same, I cannot verify other details. And I am using NextJS 14.0.4 app router. I have tried version 13.x, but I have not tried the Page router yet.

Pai-Po commented 6 months ago

By default the - onNewMessage is triggering an attempt to render but it should theoretically not call the onRender function. I made a small change in the code and deployed it to deep-chat-dev and deep-chat-react-dev packages version 9.0.102 which will stop this onNewMessage from attempting to re-render the component. These packages work exactly the same as core ones, except their names are different. Let me know if this helps you.

Thank you so much for helping me provide a temporary solution, but I want to find the root cause. I must ensure the stability of my code.

OvidijusParsiunas commented 6 months ago

When a new property is set in Deep Chat, the attemptRender method in RenderControl will always trigger to apply the latest changes when re-rendering the component. The setTimeout is used to prevent redundant renders when multiple properties are set at the same time, hence in a way it waits for all the properties to be set before activating the onRender method.

By looking at your changes, I am not sure if they yield much of a benefit. The root problem you have is that your request properties don't make it to the component on the first render attempt, hence they are not applied (the reason why we don't use the request property in further renders is to prevent the component from reconnecting to a service every time re-rendering occurs). However, let me try another fix.

Also, I wanted to ask did the changes in 9.0.102 work?

OvidijusParsiunas commented 6 months ago

I made some more changes, can you try version 9.0.103.

Pai-Po commented 6 months ago

I made some more changes, can you try version 9.0.103.

I have tested it, and it worked. Thank you again.

OvidijusParsiunas commented 6 months ago

That is good to hear, I will include this change in tomorrow's release.

OvidijusParsiunas commented 6 months ago

This bug fix has now been released in deep-chat and deep-chat-react package version 1.4.6.

Pai-Po commented 6 months ago

This bug fix has now been released in deep-chat and deep-chat-react package version 1.4.6.

Thank you.