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 index.d.ts #343

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add index.d.ts #343

wants to merge 1 commit into from

Conversation

antonok-edm
Copy link
Collaborator

closes #340

@darahe does this look good to you?

thanks to DefinitelyTyped/DefinitelyTyped#65416
for the initial version
Copy link

@darahe darahe left a comment

Choose a reason for hiding this comment

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

Left some comments, everything else looks good

STANDARD: string;
};

export const RuleTypes: {
Copy link

Choose a reason for hiding this comment

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

I guess it should be an enum or union type. I do not know which will be compatible with the Rust library. At least it is enum here: https://docs.rs/adblock/latest/adblock/lists/enum.RuleTypes.html

For now, it is an object with all required fields and it can not be used to parse only some types of rules

addFilters(rules: string[], opts?: ParseOptions): FilterListMetadata;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, after some reading (1, 2), my understanding is that enums shouldn't be declared in a .d.ts file.

This seems correct to me because it's describing the shape of (await import('adblock-rs')).RuleTypes, which basically behaves like a struct with those 3 fields.

Perhaps there is an issue with using rule_types: typeof RuleTypes in ParseOptions instead? I think it needs to be "field of" rather than typeof, but I don't know how to express that.

Copy link

Choose a reason for hiding this comment

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

Maybe you can use keyof typeof RyleTypes or union type like here:
#343 (comment)

export class Engine {
constructor(rules: FilterSet, debug: boolean);
addResource(resource: Resource): boolean;
check(url: string, source_url: string, request_type: string, debug?: false): boolean;
Copy link

Choose a reason for hiding this comment

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

Also, request_type is enum in Rust. So, I think it should be enum here too.

https://docs.rs/adblock/latest/adblock/request/enum.RequestType.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

technically, although the enum isn't currently used in any public-facing Rust API. Those functions just accept string arguments. I do want to change that but it'll have to be left to a breaking change in the future.

Copy link

Choose a reason for hiding this comment

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

Maybe we can deal with union type here:

type RequestType =
  | 'beacon'
  | 'csp'
  | 'document'
  | 'dtd'
  | 'fetch'
  | 'font'
  | 'image'
  | 'media'
  | 'object'
  | 'other'
  | 'ping'
  | 'script'
  | 'stylesheet'
  | 'subdocument'
  | 'websocket'
  | 'xlst'
  | 'xmlhttprequest';

For reference see different implementations from other adblockers:

https://github.com/AdguardTeam/tsurlfilter/blob/master/packages/tsurlfilter/src/request-type.ts#L6-L39

https://github.com/ghostery/adblocker/blob/0eb03a3f412c0764469f7561cf5f734eb4df4c66/packages/adblocker/src/request.ts#L58-L71

constructor(rules: FilterSet, debug: boolean);
addResource(resource: Resource): boolean;
check(url: string, source_url: string, request_type: string, debug?: false): boolean;
check(url: string, source_url: string, request_type: string, debug: true): BlockerResult;
Copy link

Choose a reason for hiding this comment

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

Nit: we can pass an object (aka named parameters) instead of passing 4 parameters separately, and then we will have a better user experience.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds like that's a breaking change as well, so I'll leave it as-is for now

@darahe
Copy link

darahe commented Mar 18, 2024

@antonok-edm If you need some help to finish this PR you can ping me here

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.

Wrong types for js bindings
2 participants