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

Broken typescript example #15

Merged
merged 7 commits into from
Mar 8, 2020
Merged

Conversation

vojty
Copy link

@vojty vojty commented Feb 18, 2020

Run ./node_modules/.bin/tsc --noEmit

@jamuhl
Copy link
Member

jamuhl commented Feb 18, 2020

@Vovan-VE would you mind checking this PR - types just were added by your last PR - so a little help would help - as I myself do not use typescript at all (and do not really care for it)

@vojty
Copy link
Author

vojty commented Feb 18, 2020

I'll look into it, maybe @Vovan-VE could help me

@vojty
Copy link
Author

vojty commented Feb 18, 2020

remaining errors:

10:20 $ ./node_modules/.bin/tsc
index.d.ts:24:24 - error TS2713: Cannot access 'IcuFormats.number' because 'IcuFormats' is a type, but not a namespace. Did you mean to retrieve the type of the property 'number' in 'IcuFormats' with 'IcuFormats["number"]'?

24         [key: string]: IcuFormats.number;
                          ~~~~~~~~~~~~~~~~~

index.d.ts:59:20 - error TS2430: Interface 'IcuInstance' incorrectly extends interface 'ThirdPartyModule'.
  Types of property 'type' are incompatible.
    Type '"i18nFormat"' is not assignable to type '"3rdParty"'.

59   export interface IcuInstance extends ThirdPartyModule {
                      ~~~~~~~~~~~


Found 2 errors.

@Vovan-VE
Copy link
Contributor

Hello.

The 1st error: I just got IcuFormats from here #12 (comment). I don't know any details about this structure. Maybe @dyniper provide some details? Replacing IcuFormats.number with IcuFormats["number"] produce more errors due to recursion.

The 2nd error is then children interface IcuInstance.type defined as "i18nFormat" is not assignable to parent interface ThirdPartyModule.type which is defined as "3rdParty". Here I just follow literally to:

i18next-icu/src/index.js

Lines 11 to 13 in f46d0bf

class ICU {
constructor(options) {
this.type = 'i18nFormat';

If parent interface defined so, then it means you cannot change type like this.

@Vovan-VE
Copy link
Contributor

I suppose, Intl.NumberFormatOptions should be used instead of this crafted object

i18next-icu/index.d.ts

Lines 15 to 19 in f46d0bf

[styleName: string]: {
style?: "currency" | "percent" | "decimal";
currency?: string;
currencyDisplay?: "name" | "symbol" | "code";
minimumIntegerDigits?: 1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19|20|21;

@Vovan-VE
Copy link
Contributor

...and Intl.DateTimeFormatOptions for two similar objects below there

i18next-icu/index.d.ts

Lines 28 to 31 in f46d0bf

[styleName: string]: {
weekday?: "narrow" | "short" | "long";
era?: "narrow" | "short" | "long";
year?: "numeric" | "2-digit";

i18next-icu/index.d.ts

Lines 42 to 45 in f46d0bf

[styleName: string]: {
hour?: "numeric" | "2-digit";
minute?: "numeric" | "2-digit";
second?: "numeric" | "2-digit";

@vojty
Copy link
Author

vojty commented Feb 18, 2020

@Vovan-VE feel free to push changes :)

@Vovan-VE
Copy link
Contributor

I also tried to comment out type

this.type = 'i18nFormat';

but I cannot run tests even before this.

@Vovan-VE
Copy link
Contributor

npm test sais that Promise is not defined. Probably, some dependencies are outdated.
node 12.13

18 02 2020 18:31:43.294:INFO [framework.browserify]: bundle built
18 02 2020 18:31:43.305:INFO [karma]: Karma v1.4.1 server started at http://0.0.0.0:9876/
18 02 2020 18:31:43.305:INFO [launcher]: Launching browser PhantomJS with unlimited concurrency
18 02 2020 18:31:43.312:INFO [launcher]: Starting browser PhantomJS
18 02 2020 18:31:43.531:INFO [PhantomJS 2.1.1 (Linux 0.0.0)]: Connected on socket EBotkANsv9VLpnroAAAA with id 43711429

  icu format
    basic parse
      ✓ should parse
      ✓ should parse (AR)
    with formatter
      ✓ should parse with custom format
    with i18next
      ✗ "before all" hook
	Can't find variable: Promise
	defer@/tmp/node_modules/i18next/dist/cjs/i18next.js:183:0 <- /tmp/454badde459db454f68b813137afd542.browserify:436:28
	init@/tmp/node_modules/i18next/dist/cjs/i18next.js:1923:0 <- /tmp/454badde459db454f68b813137afd542.browserify:2176:27
	/tmp/test/icu.spec.js:91:14 <- /tmp/454badde459db454f68b813137afd542.browserify:9046:48


PhantomJS 2.1.1 (Linux 0.0.0): Executed 4 of 4 (1 FAILED) (0.076 secs / 0.014 secs)
TOTAL: 1 FAILED, 3 SUCCESS


1) "before all" hook
     icu format with i18next
     Can't find variable: Promise
defer@/tmp/node_modules/i18next/dist/cjs/i18next.js:183:0 <- /tmp/454badde459db454f68b813137afd542.browserify:436:28
init@/tmp/node_modules/i18next/dist/cjs/i18next.js:1923:0 <- /tmp/454badde459db454f68b813137afd542.browserify:2176:27
/tmp/test/icu.spec.js:91:14 <- /tmp/454badde459db454f68b813137afd542.browserify:9046:48


npm ERR! Test failed.  See above for more details.

@vojty
Copy link
Author

vojty commented Feb 18, 2020

I did some major changes in devDependencies (upgraded babel and replaced karma with jest), hope it's ok.

@vojty vojty marked this pull request as ready for review February 18, 2020 12:13
@vojty
Copy link
Author

vojty commented Feb 20, 2020

@jamuhl fyi ready for review

@jamuhl
Copy link
Member

jamuhl commented Feb 20, 2020

@Vovan-VE looks ok to me - if ok for you I will merge an pubish

@Vovan-VE
Copy link
Contributor

Yes, surely

@vojty
Copy link
Author

vojty commented Mar 8, 2020

bump

@jamuhl jamuhl merged commit a83243c into i18next:master Mar 8, 2020
@jamuhl
Copy link
Member

jamuhl commented Mar 8, 2020

published in i18next-icu@1.2.1

@vojty
Copy link
Author

vojty commented Mar 8, 2020

thanks :)

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.

None yet

3 participants