-
Notifications
You must be signed in to change notification settings - Fork 603
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
Add type definitions via index.d.ts #2514
base: master
Are you sure you want to change the base?
Conversation
This is a huge step in the right direction, thank you @steveszc! |
Sorry to bring it up only now @steveszc 🙏 I myself am not well informed about consuming Typescript addons in ember projects. Could you please document the steps to include Another question would be - would it make sense in your opinion, to make SessionService argument optional? import SessionService from 'ember-simple-auth/services/session';
interface SessionData {
authenticated: Record<string, unknown>
}
export default class Application extends Controller {
@service declare session: SessionService<SessionData>;
} This particular problem potentially isn't huge because the Would you consider it the expect API? Or maybe the recommendation should be to extend the ESA service instead? // user land
// app/services/session.ts
import EsaSessionService from 'ember-simple-auth/services/session';
type Data = Record<string, unknown>;
export interface SessionData {
authenticated: Data;
}
export default class UserLandSessionService from EsaSessionService<SessionData> {}
// DO NOT DELETE: this is how TypeScript knows how to look up your services.
declare module '@ember/service' {
interface Registry {
'session': UserLandSessionService;
}
} import Component from '@glimmer/component';
import SessionService from 'user-land/services/session';
import { service } from '@ember/service';
export class MyComponent extends Component {
@service declare session: SessionService;
} |
Dismissing approve for now. I'm open to merging this, however I ended up having some questions after I played around with it for a bit 🙏
In general, types should be imported where they need to be consumed. I think we would typically import them in specific files, where needed, rather than globally. |
I second this ☝️
The steps I followed are:
@BobrImperator would be nice if you could give this another look. I'm working on migrating our app to typescript and this PR would be a real help. |
Thanks for participating in the discussion 💯 I'd like ESA to include the recommended setup steps in the readme and we should be ready to go. |
I'm happy to update the readme with some instructions in this PR. |
@steveszc Would be nice to get this one in. I added some docs in steveszc#1. Please 🙏 review and merge so @BobrImperator can add these type defs in. |
I made the requested changes @steveszc Please merge if it's OK and then @BobrImperator can merge this PR. |
Any chance this could be merged soon? |
+1 |
Any news? |
anything holding this up? |
@BobrImperator @marcoow any chance we could merge this? It would be super helpful! |
From my perspective this seems fine, the provided types look good as well. This needs to be rebased and then adapted for the V2 addon structure. Would be nice to have a guide directly in the readme on how to enable the types in user projects. |
This isn't needed.
|
restore(data: Data): Promise<unknown>; | ||
authenticate(...args: unknown[]): Promise<unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Promise<unknown>
here can actually be Promise<SessionData<T>>
.
I don't know how to make sure SessionService<T>.data
and these return values are in synced though.
import Evented from '@ember/object/evented'; | ||
import Service from '@ember/service'; | ||
import type BaseSessionStore from 'ember-simple-auth/session-stores/base'; | ||
import type Transition from '@ember/routing/-private/transition'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could now be imported from the public transition type:
-import type Transition from '@ember/routing/-private/transition';
+import type Transition from '@ember/routing/transition'
} | ||
|
||
declare module 'ember-simple-auth/services/session' { | ||
import Evented from '@ember/object/evented'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the imports could be import type
, i guess:
-import Evented from '@ember/object/evented';
+import type Evented from '@ember/object/evented';
-import Service from '@ember/service';
+import type Service from '@ember/service';
-import EmberObject from '@ember/object';
+import type EmberObject from '@ember/object';
-import BaseSessionStore from 'ember-simple-auth/session-stores/base';
+import type BaseSessionStore from 'ember-simple-auth/session-stores/base';
-import CookieSessionStore from 'ember-simple-auth/session-stores/base';
+import type CookieSessionStore from 'ember-simple-auth/session-stores/base';
-import BaseAuthenticator from 'ember-simple-auth/authenticators/base';
+import type BaseAuthenticator from 'ember-simple-auth/authenticators/base';
-import SimpleAuthSessionService from 'ember-simple-auth/services/session';
+import type SimpleAuthSessionService from 'ember-simple-auth/services/session';
This PR adds a comprehensive index.d.ts that adds type definitions for all public classes in ember-simple-auth.
This types are currently in-use internally at CrowdStrike.
While this approach has the benefit of finally landing "official" types, these types must be hand maintained as changes are made to the implementation itself. A better approach might be native TS implementation or JSDoc types, but this is better than nothing and will give ember-simple-auth a base of types to build upon and improve.
fixes #2064