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

Export Additional Functions #41

Merged
merged 1 commit into from
Jun 25, 2021
Merged

Export Additional Functions #41

merged 1 commit into from
Jun 25, 2021

Conversation

JAD3N
Copy link
Contributor

@JAD3N JAD3N commented Jun 18, 2021

A quick fix for the typings.

Q A
Bug fix? yes
New feature? no
Deprecations? no
License MIT

A quick fix for the typings.
@AKorezin
Copy link
Contributor

Maybe we should use dts-gen result as a base instead?

export = whoiser;

declare function whoiser(query: any, options: any): any;

declare namespace whoiser {
    function allTlds(): any;

    function asn(query: any, { host = null, timeout = 15000, raw = false }: any): any;

    function domain(domain: any, { host = null, timeout = 15000, follow = 2, raw = false }: any): any;

    function firstResult(whoisResults: any): any;

    function ip(query: any, { host = null, timeout = 15000, raw = false }: any): any;

    function query({ host = null, port = 43, timeout = 15000, query = '', querySuffix = '\r\n' }: any): any;

    function tld(query: any, { timeout = 15000, raw = false }: any): any;

}

Or maybe this should be done like this?

declare module 'whoiser' {

	...

	export default whoiser;
	export {
		tld,
		allTlds,
		ip,
		query,
		whoiser,
		WhoisSearchResult,
		Options,
		OptionsGeneric,
		OptionsIp,
		OptionsDomain,
		OptionsTld,
		OptionsQuery,
		OptionsAsn,
	};
}

@JAD3N
Copy link
Contributor Author

JAD3N commented Jun 21, 2021

Maybe we should use dts-gen result as a base instead?

export = whoiser;

declare function whoiser(query: any, options: any): any;

declare namespace whoiser {
    function allTlds(): any;

    function asn(query: any, { host = null, timeout = 15000, raw = false }: any): any;

    function domain(domain: any, { host = null, timeout = 15000, follow = 2, raw = false }: any): any;

    function firstResult(whoisResults: any): any;

    function ip(query: any, { host = null, timeout = 15000, raw = false }: any): any;

    function query({ host = null, port = 43, timeout = 15000, query = '', querySuffix = '\r\n' }: any): any;

    function tld(query: any, { timeout = 15000, raw = false }: any): any;

}

Or maybe this should be done like this?

declare module 'whoiser' {

	...

	export default whoiser;
	export {
		tld,
		allTlds,
		ip,
		query,
		whoiser,
		WhoisSearchResult,
		Options,
		OptionsGeneric,
		OptionsIp,
		OptionsDomain,
		OptionsTld,
		OptionsQuery,
		OptionsAsn,
	};
}

I believe the inline exports are bit more intuitive than exporting at the end of the file and more preferable than using a namespace. In my project where I'm using whoiser, I override the typings using the ones proposed, so I'm sure there isn't any problem with them specifically.

I'm okay with changing the pull request if the direction is clear, please let me know how to proceed.

@AKorezin
Copy link
Contributor

I'm not a ts programmer, so that was my mistake.
@JAD3N, your pr looks good for me. I think there's no problem with inline exports.

Also it seems modules are better then namespace according to this document https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html.
I don't know why but it says we recommended modules over namespaces in modern code.

@JAD3N JAD3N closed this Jun 24, 2021
@JAD3N JAD3N deleted the patch-1 branch June 24, 2021 08:18
@AKorezin
Copy link
Contributor

@JAD3N what have happened? I think this PR is 👍 and have to be merged into the upstream.

@AndreiIgna can you merge this PR? You can do it locally if it's not possible with web.
I can create a new one if it's required.

@JAD3N JAD3N restored the patch-1 branch June 25, 2021 09:32
@JAD3N JAD3N reopened this Jun 25, 2021
@AndreiIgna AndreiIgna merged commit 03045c7 into LayeredStudio:master Jun 25, 2021
@AndreiIgna
Copy link
Member

Thanks for the contributions @JAD3N @AKorezin. This is merged and will be released on npm in 1-2 days

@JAD3N JAD3N deleted the patch-1 branch June 25, 2021 15:35
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.

3 participants