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

Types are not included properly in the output package #37

Closed
christopher-caldwell opened this issue Dec 1, 2022 · 8 comments
Closed

Comments

@christopher-caldwell
Copy link

It looks like types are included in the source, but not in what actually gets published.

Here's what I see in node_modules:
Screen Shot 2022-12-01 at 2 46 25 PM

And when I try to import it
Screen Shot 2022-12-01 at 2 48 32 PM

I think the fix here would be to include the types in the bundle, and declare them in the package.json:

"main": "slack-notify.js",
"types": "index.d.ts",
@KaydenMiller
Copy link

It also looks like many of the interfaces in the types are not exported and so cannot easily be used via import { SlackNotify } from 'slack-notify'; as well. This means that in my project we have to mark the generated notification objects as any in order to not have errors.

@andrewchilds
Copy link
Owner

@christopher-caldwell, please update to the latest version.

@KaydenMiller, could you clarify what you're seeing and what you're expecting to happen? import { SlackNotify } from 'slack-notify' shouldn't work because the ESM version doesn't provide a named export. Do you mean to say import SlackNotify from 'slack-notify'?

@KaydenMiller
Copy link

KaydenMiller commented Dec 2, 2022

Yeah no problem, and maybe i'm just going about this wrong. As far as I can tell in the https://github.com/andrewchilds/slack-notify/blob/main/src/index.d.ts file there are several interfaces, specifically interface SlackNotify extends SlackNofifier which is not exported (though having every interface would be nice). This causes an issue when you try to reference it as a type to be used in a block like below.

This block is in TS:

// Given the default and the interface name are the same I would need to alias one
// but for the purpose of this im not this would probably look like 
import SlackNotify as NotifyFactory, { SlackNotify } from 'slack-notify'; // could be wrong on the syntax here

import SlackNotify, { SlackNotify } from 'slack-notify'; // this will cause an issue because the SlackNotify interface from the index.d.ts is not exported and so ts will not allow you to import it
const channel: SlackNotify = SlackNotify('hook-url-other');

const slackChannels: any = {     // would prefer not to use any here
  'alerts': SlackNotify(
    'hook-url1'
  ),
  'monitoring': SlackNotify(
    'hook-url2'
  ),
  'internal-notices': SlackNotify(
    'hook-url3'
  ),
  'link-notifications': SlackNotify(
    'hook-url4'
  ),
}

// above const would probably look more like
const slackChannels: { [key: string]: SlackNotify } = { }; // or something like that

I don't think the default is a problem (as i could just alias it) but I think it would be very useful to be able to import the types that are used for reference. And perhaps there is a way to do this, as TS isn't the main language I work in, but I haven't found one yet.

@KaydenMiller
Copy link

KaydenMiller commented Dec 2, 2022

The types work implicitly which works in a lot of situations but they don't appear to work if you want to explicitly reference them.

If this does end up being an issue and one you want changed, I'm happy to make a PR for it.

@christopher-caldwell
Copy link
Author

@christopher-caldwell, please update to the latest version.

Yep! Sorry about that. All good for me.

@andrewchilds
Copy link
Owner

Thanks @KaydenMiller, that's very helpful context. I don't use types in my own work, so I'm leaning on people that do to help keep this library working in every use case.

That said, if this is just a matter of adding export to every interface in the type definition file, that seems straightforward and low risk.

Here's what the type definition file would look like if we exported every interface and the factory function, but kept the default export:

declare module 'slack-notify' {
  export interface SlackNotifier {
    send(args: string | SendArgs): Promise<void>;
  }

  export interface SlackNotify extends SlackNotifier {
    extend(args: string | SendArgs): SlackNotifier;
    success(args: string | SendArgs): Promise<void>;
    bug(args: string | SendArgs): Promise<void>;
    alert(args: string | SendArgs): Promise<void>;
  }

  export interface SendArgs {
    text: string;
    blocks?: any;
    channel?: string;
    icon_url?: string;
    icon_emoji?: string;
    unfurl_links?: number;
    username?: string;
    attachments?: SendAttachment[];
    fields?: { [key: string]: string };
  }

  export interface SendAttachment {
    fallback: string;
    color: string;
    fields?: SendAttachmentField[];
  }

  export interface SendAttachmentField {
    title: string;
    value: string;
    short: boolean;
  }

  export function SlackNotifyFactory(webhookUrl: string): SlackNotify;

  export default SlackNotifyFactory;
}

And the example TS code I assume would be:

import { SlackNotifyFactory, SlackNotify } from 'slack-notify';

const generalChannel: SlackNotify = SlackNotifyFactory('hook-url1');

const otherChannels: { [key: string]: SlackNotify } = {
  alerts: SlackNotifyFactory('hook-url2'),
  monitoring: SlackNotifyFactory('hook-url3'),
  // etc.
};

Does this look right to you?

@KaydenMiller
Copy link

KaydenMiller commented Dec 2, 2022

Yeah that looks pretty good, i think with keeping the default import the usage would be

// When using the default import
import SlackNotifyFactory, { SlackNotify } from 'slack-notify';

// When using the standard export (not sure what that is called)
import { SlackNotifyFactory, SlackNotify } from 'slack-notify';

as the default import is not included inside the braces. Otherwise yea i think that should work. Though with what you have I think you would actually be able to do either as you are exporting both a default export and the standard export.

In short, yeah i think the changes you have outlined work work great for what I was describing.

@andrewchilds
Copy link
Owner

@KaydenMiller ok this is updated and published to v2.0.6. I forgot to reference this issue in the commit message but the change is in f4bb1a8. Thanks for your help!

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

No branches or pull requests

3 participants