catamphetamine / libphonenumber-js

A simpler (and smaller) rewrite of Google Android's libphonenumber library in javascript
https://catamphetamine.gitlab.io/libphonenumber-js/
MIT License
2.77k stars 217 forks source link

Fix missing type definitions when importing from subpackages #290

Closed lucsky closed 5 years ago

lucsky commented 5 years ago

Explicitely point to the main type definition file from the subpackages package.info to allow types to be used when doing import 'libphonenumber-js/{min,max,mobile}.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 72d5d7bcb47e9120f95579c50513b517d026c93c on lucsky:master into 4430b627b5eeeb3b9cb8efcffa4263c3bf191289 on catamphetamine:master.

catamphetamine commented 5 years ago

I based my commit on your idea. See if it works: https://github.com/catamphetamine/libphonenumber-js/commit/20e95de3a25386dd33b0a6a3856e4062bc3ad780 I haven't published it yet. If you say that it doesn't break and that it seems to work then I'll publish it.

catamphetamine commented 5 years ago

@lucsky No need to do any extensive testing. Just put these files in your node_modules/libphonenumber-js directory and tell me if it doesn't break everything. If it doesn't break your code then I'll publish it.

lucsky commented 5 years ago

It does break it, because the index.d.ts that you put in min (and which gets used in mobile and max) does not re-export the base types imported at the beginning of the file (CountryCallingCode, CountryCode, NumberFound, and PhoneNumber).

Adding the following line (and any other imported types that you might want to expose publicly) to min/index.d.ts makes it work (at least for me 😄):

export { CountryCallingCode, CountryCode, NumberFound, PhoneNumber };

Thanks!

catamphetamine commented 5 years ago

I see, so re-exporting base types is also required then.

What about importing them from “libphonenumber-js”? That doesn’t look clean to me. Can it be a relative path? Something like: from “../types.d.ts” or something like that? Maybe without extension? Can you try it?

On Fri, 11 Jan 2019 at 01:33, Luc Heinrich notifications@github.com wrote:

It does break it, because the index.d.ts that you put in min (and which gets used in mobile and max) does not re-export the base types imported at the beginning of the file (CountryCallingCode, CountryCode, NumberFound, and PhoneNumber).

Adding this line (and any other imported types that you might want to expose publicly) to min/index.d.ts makes it work (at least for me 😄):

export { CountryCallingCode, CountryCode, NumberFound, PhoneNumber };

Thanks!

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/catamphetamine/libphonenumber-js/pull/290#issuecomment-453281180, or mute the thread https://github.com/notifications/unsubscribe-auth/AAdH7xnaY9x2CIrbzPrEeOVZzf2zGMfWks5vB7-ygaJpZM4Z6Wf6 .

lucsky commented 5 years ago

What do you mean by "importing them from “libphonenumber-js" ? From the client ? Or from the type definition file ? Ideally there should only be one entry (import) point.

catamphetamine commented 5 years ago

@lucsky Well, currently it's

import { CountryCallingCode, CountryCode, NumberFound, PhoneNumber } from 'libphonenumber-js';

It was written by someone. Can it be replaced with something like:

import { CountryCallingCode, CountryCode, NumberFound, PhoneNumber } from '../types';

Where types.d.ts would be a new file containing only type definitions (in the root of the module folder).

lucsky commented 5 years ago

That's already how it works right now:

import { CountryCallingCode, CountryCode, NumberFound, PhoneNumber } from 'libphonenumber-js';

picks up the index.d.ts file in the root folder.

catamphetamine commented 5 years ago

@lucsky Yeah, I mean, why does it have to import from "package-name"? Why isn't it just "from './index.d.ts'?

lucsky commented 5 years ago

I see what you mean. In that case you can selectively import and re-export types like this:

export type CountryCallingCode = import('../index.d.ts').CountryCallingCode;
export type CountryCode = import('../index.d.ts'). CountryCode;
export type NumberFound = import('../index.d.ts'). NumberFound;
export type PhoneNumber = import('../index.d.ts'). PhoneNumber;
...
catamphetamine commented 5 years ago

Ok, and what about this:

import { CountryCallingCode, CountryCode, NumberFound, PhoneNumber } from '../types.d.ts';
export{ CountryCallingCode, CountryCode, NumberFound, PhoneNumber };

Would it work? I guess it would.

catamphetamine commented 5 years ago

@lucsky Can you put the types attached to this comment into your node_modules/libphonenumber-js folder and see if it breaks your code? If it doesn't then I'll release them. typescript.zip

lucsky commented 5 years ago

@catamphetamine yeah that works, but why adding yet another types.d.ts file when index.d.ts already exists ?

catamphetamine commented 5 years ago

but why adding yet another types.d.ts file when index.d.ts already exists ?

Yeah, I guess. Refactored index.d.ts and custom.d.ts: they now import types from ./types.d.ts too. See if it didn't break your app (archive attached). typescript.zip

lucsky commented 5 years ago

All good. Thanks Nikolay 😄