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

Would the argument for getThreadContext and saveThreadContext be unnecessary? #2319

Closed
ttexan1 opened this issue Nov 4, 2024 · 6 comments · Fixed by #2325
Closed

Would the argument for getThreadContext and saveThreadContext be unnecessary? #2319

ttexan1 opened this issue Nov 4, 2024 · 6 comments · Fixed by #2325
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch

Comments

@ttexan1
Copy link

ttexan1 commented Nov 4, 2024

@slack/bolt version

^4.1.0

Your App and Receiver Configuration

...

Node.js runtime version

v20.16.0

Steps to reproduce:

When I use default thread context store functionalities.

threadStarted: async ({ event, setSuggestedPrompts, saveThreadContext }) => {
    const { context } = event.assistant_thread;

    try {
      await saveThreadContext();
      ...

following typescript error occures
Argument of type 'number' is not assignable to parameter of type 'AssistantConfig'.ts(2345)

Expected 1 arguments, but got 0.ts(2554)
AssistantThreadContextStore.d.ts(7, 36): An argument for 'args' was not provided.

Expected result:

bolt-js/src/Assistant.ts

Lines 32 to 34 in 015a918

interface AssistantUtilityArgs {
getThreadContext: GetThreadContextFn;
saveThreadContext: SaveThreadContextFn;

getThreadContext has the same signature as the get method of threadContextStore

bolt-js/src/Assistant.ts

Lines 177 to 178 in 015a918

preparedArgs.getThreadContext = () => threadContextStore.get(args);
preparedArgs.saveThreadContext = () => threadContextStore.save(args);

but the implementation of getThreadContext requires no arguments. Wouldn’t it be appropriate to remove the argument from type definition?
(The same could be said for saveThreadContext.)

@ttexan1 ttexan1 changed the title (Set a clear title describing your question) Would the argument for getThreadContext and saveThreadContext be unnecessary? Nov 4, 2024
@filmaj filmaj added needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info and removed untriaged labels Nov 6, 2024
@filmaj
Copy link
Contributor

filmaj commented Nov 6, 2024

Hello! I tried reproducing this issue and was unable to. Can you provide a sample that reproduces the issue? If it's a typescript issue, a sample project that reproduces this issue would be helpful that includes e.g. a package.json with typescript version, your tsconfig.json file, and so on.

@ttexan1
Copy link
Author

ttexan1 commented Nov 7, 2024

@filmaj
Thank you for checking.
I’m sorry, I realized I had made a mistake in the most important part—the error message content—so I’ve corrected it.
Expected 1 arguments, but got 0.ts(2554) is the correct message.

image

What I’d like to confirm is that there is a discrepancy between the implementation and the type definitions, causing an error on the caller’s side.

typescript version is ^5.5.4 and my tsconfig.json is here.

{
  "compilerOptions": {
    "target": "es2016",
    "module": "commonjs",
    "rootDir": "./src",
    "outDir": "./dist",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "noImplicitReturns": true,
    "noFallthroughCasesInSwitch": true,
    "noUncheckedIndexedAccess": true,
    "skipLibCheck": true
  },
  "include": [
    "src/**/*"
  ],
  "exclude": [
    "node_modules",
    "**/*.spec.ts"
  ]
}

@filmaj
Copy link
Contributor

filmaj commented Nov 7, 2024

Thanks! I can reproduce the issue. Will investigate the issue.

@filmaj filmaj added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch and removed needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info labels Nov 7, 2024
@filmaj
Copy link
Contributor

filmaj commented Nov 7, 2024

I've created a draft PR with a test that exhibits this issue: #2324

@filmaj
Copy link
Contributor

filmaj commented Nov 7, 2024

While we work through how to address this and what the proper fix is, a temporary workaround is to put a // @ts-expect-error comment above the relevant call that is causing a compilation issue. We should have this resolved soon, and I can confirm that this is a purely type-checking/compilation issue and does not affect runtime behaviour or correctness.

@ttexan1
Copy link
Author

ttexan1 commented Nov 8, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants