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

Add type definitions via index.d.ts #2514

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

steveszc
Copy link

@steveszc steveszc commented Feb 8, 2023

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

@RobbieTheWagner
Copy link
Contributor

This is a huge step in the right direction, thank you @steveszc!

BobrImperator
BobrImperator previously approved these changes Feb 28, 2023
@BobrImperator
Copy link
Collaborator

BobrImperator commented Mar 3, 2023

Sorry to bring it up only now @steveszc 🙏

I myself am not well informed about consuming Typescript addons in ember projects.
So I went to setup a project to test this out and I had to specifically import the types in my types/global.d.ts file.

image

Could you please document the steps to include ember-simple-auth types in user projects?

Another question would be - would it make sense in your opinion, to make SessionService argument optional?
At the moment users will be forced to provide the type and carry it around e.g.

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 session service usually is abstracted or extended, however that'd also need to be documented.

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;
}

@BobrImperator BobrImperator dismissed their stale review March 3, 2023 11:55

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 🙏

@RobbieTheWagner
Copy link
Contributor

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.

@colenso
Copy link

colenso commented Mar 23, 2023

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 ☝️
As for:

Could you please document the steps to include ember-simple-auth types in user projects?

The steps I followed are:

  1. Include it in the tsconfig.json under paths like so:
"paths: {
    "ember-simple-auth/*": [
        "node_modules/ember-simple-auth"
      ],
}
  1. Import the definition for the type that you need where you need it. For eg: If I have a session-auth.js service that uses the session service offered by ember-simple-auth, I'd import it and use it something like this:
session-auth.js
import { inject as service } from '@ember/service';
import Session from 'ember-simple-auth/services/session';

export default class SessionAuthService extends Service {

    @service declare session: Session<{}>

}

@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.

@BobrImperator
Copy link
Collaborator

Thanks for participating in the discussion 💯
This makes sense to me 👍

I'd like ESA to include the recommended setup steps in the readme and we should be ready to go.

@steveszc
Copy link
Author

I'm happy to update the readme with some instructions in this PR.

@colenso
Copy link

colenso commented Mar 28, 2023

@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.

@steveszc
Copy link
Author

@colenso I provided some feedback here. A couple changes are needed to get it merged.

@colenso
Copy link

colenso commented Apr 6, 2023

@colenso I provided some feedback here. A couple changes are needed to get it merged.

I made the requested changes @steveszc Please merge if it's OK and then @BobrImperator can merge this PR.

@RobbieTheWagner
Copy link
Contributor

Any chance this could be merged soon?

@GerritSommer
Copy link

+1

@BryanCrotaz
Copy link
Contributor

Any news?

@NullVoxPopuli
Copy link

anything holding this up?

@RobbieTheWagner
Copy link
Contributor

@BobrImperator @marcoow any chance we could merge this? It would be super helpful!

@BobrImperator
Copy link
Collaborator

BobrImperator commented Oct 6, 2023

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.
Where as I understand the index.d.ts will need to be copied to dist/ during build.

Would be nice to have a guide directly in the readme on how to enable the types in user projects.
But that can be added separately.

@NullVoxPopuli
Copy link

Where as I understand the index.d.ts will need to be copied to dist/ during build.

This isn't needed.
To have the d.ts picked up, it only needs:

  • to be present in the npm package (covered by package.json#files or the inverse of .npmignore)
  • and have types/exports.*.types point at the d.ts file.
    • or be named index.d.ts in the root of the package (index files are automatically identified)

Comment on lines +36 to +37
restore(data: Data): Promise<unknown>;
authenticate(...args: unknown[]): Promise<unknown>;
Copy link

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';
Copy link

@johanrd johanrd Jan 19, 2024

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';
Copy link

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';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript definitions for injected session service
10 participants