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

refactor: clean core experiment #938

Closed
wants to merge 8 commits into from
Closed

refactor: clean core experiment #938

wants to merge 8 commits into from

Conversation

freelerobot
Copy link
Contributor

@freelerobot freelerobot commented Dec 11, 2023

This PR (originally an experiment) refactors types into clean entities

namchuai

This comment was marked as resolved.

core/src/extension.ts Outdated Show resolved Hide resolved
@louis-jan
Copy link
Contributor

louis-jan commented Dec 11, 2023

@0xSage, (abstract) classes are available at runtime, while interfaces are compile-time only. We built the core SDK to allow the Extension Builder and App to inherit/use it, mostly at runtime. There are a bunch of type checking to let app can poll correct extensions to invoke their function at runtime.

let x: any;
// IThreadExtension is an interface 
if (x instanceof IThreadExtension) { // Error: 'ThreadExtension' only refers to a type, but is being used as a value here.

}
// ThreadExtension is an abstract class
if (x instanceof ThreadExtension) { // OK

}

@freelerobot
Copy link
Contributor Author

@0xSage, (abstract) classes are available at runtime, while interfaces are compile-time only. We built the core SDK to allow the Extension Builder and App to inherit/use it, mostly at runtime. There are a bunch of type checking to let app can poll correct extensions to invoke their function at runtime.

Yes, I realized this while refactoring Events (still in progress). Reverting the ai-kit extensions back to abstract classes.
Maybe ignore this PR for a few hours, it is very POC 😓

@freelerobot freelerobot changed the title refactor: clean core refactor: clean core (WIP) Dec 11, 2023
@freelerobot freelerobot changed the title refactor: clean core (WIP) refactor: clean core (WIP; Very Experimental) Dec 11, 2023
@freelerobot freelerobot changed the title refactor: clean core (WIP; Very Experimental) refactor: clean core (WIP & Very Experimental) Dec 11, 2023
@dan-homebrew
Copy link
Contributor

I'm really not sure ai-kit and node-kit are good names. Please reconsider

@dan-homebrew dan-homebrew added this to the Jan has Clean Architecture milestone Dec 11, 2023
@freelerobot
Copy link
Contributor Author

freelerobot commented Dec 13, 2023

@louis-jan our codebase has evolved significantly since I did this experiment (kudos to you and team).

What pieces of this experiment should actually be merged in?

What about refactoring types and interfaces into something like:

/types
  /assistant
    assistant.ts (for types, enums, globals)
    assistantInterface.ts (for interface definition)
    index.ts (simply exports types & interfaces)

such that (s.t.):
/core/extensions can just be:
export abstract class AssistantExtension extends BaseExtension implements AssistantInterface.

It may seem redundant but it decouples interfaces from extensions (and makes dep path clearer).

For any changes worthy of being incorporated, I'd like @hieu-jan to take over from here.

@louis-jan
Copy link
Contributor

louis-jan commented Dec 13, 2023

@louis-jan our codebase has evolved significantly since I did this experiment (kudos to you and team).

What pieces of this experiment should actually be merged in?

What about refactoring types and interfaces into something like:

/types
  /assistant
    assistant.ts (for types, enums, globals)
    assistantInterface.ts (for interface definition)
    index.ts (simply exports types & interfaces)

such that (s.t.): /core/extensions can just be: export abstract class AssistantExtension extends BaseExtension implements AssistantInterface.

It may seem redundant but it decouples interfaces from extensions (and makes dep path clearer).

For any changes worthy of being incorporated, I'd like @hieu-jan to take over from here.

@0xSage The types refactoring is neat. Please merge, also decouples interfaces, totally agree.

@freelerobot freelerobot changed the title refactor: clean core (WIP & Very Experimental) refactor: clean core types Dec 13, 2023
@freelerobot freelerobot changed the title refactor: clean core types refactor: clean core experiment Dec 13, 2023
@freelerobot
Copy link
Contributor Author

Closing this PR in favor of #966

@hiro-v hiro-v deleted the refactorCore branch December 20, 2023 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants