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

fix(types): getXXX methods return instances #38

Merged

Conversation

Dam-Buty
Copy link
Contributor

@Dam-Buty Dam-Buty commented Jul 1, 2024

No description provided.

Copy link

linear bot commented Jul 1, 2024

ENG-1585 TS SDK : getXXX methods should return instances

There are some inconsistencies in there, for example :

  • getStep is typed as returning a Step , but in fact it returns a plain object of shape StepFields
  • getThread is typed as returning any, returns a ThreadFields object
  • there are probably more inconsistencies

The fact that these methods don't return actionable instances (with the .send method, etc…) is a bit of a missed opportunity. In scenarios where you want to work on an already created entity, you have to do that little dance which is annoying.

Dod

  • types are fixed
  • entities are instantiated as their respective class
  • this is non-breaking for existing project which might already do the little dance. Basically just check that re-instantiating an instance is okay.

@@ -82,7 +82,7 @@ describe('End to end tests for the SDK', function () {
expect(thread.metadata).toStrictEqual({ foo: 'bar' });

const fetchedThread = await client.api.getThread(thread.id);
expect(fetchedThread.id).toBe(thread.id);
expect(fetchedThread?.id).toBe(thread.id);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only "breaking" change i could identify. Existing (erroneous) code may rely on the fact that getThread used to be typed any and not include any null checks.

@Dam-Buty Dam-Buty merged commit 6f4acfc into main Jul 2, 2024
2 checks passed
@Dam-Buty Dam-Buty deleted the damien/eng-1585-ts-sdk-getxxx-methods-should-return-instances branch August 1, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants