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

fix: TypeScript i18next import #192

Merged
merged 1 commit into from
Sep 2, 2019
Merged

fix: TypeScript i18next import #192

merged 1 commit into from
Sep 2, 2019

Conversation

marcobiedermann
Copy link
Contributor

Description

Fix TypeScript import of i18next in module declaration.

Since i18next does not provide a default export you have to import everything.
Setting the TypeScript compile option esModuleInterop is not a good practise and not an option for most projects out there.


Error stack

node_modules/i18next-express-middleware/index.d.ts:2:8 - error TS1259: Module '"/Users/biedema/Documents/GitHub/work/moovel/mobility-benefits-core/node_modules/i18next/index"' can only be default-imported using the 'esModuleInterop' flag

2 import i18next from 'i18next';
         ~~~~~~~

  node_modules/i18next/index.d.ts:988:1
    988 export = i18next;
        ~~~~~~~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

Fix TypeScript import of i18next in module declaration
@jamuhl
Copy link
Member

jamuhl commented Aug 30, 2019

@rosskevin can you check this one?

@rosskevin
Copy link

This is correct and works for both variations on esModuleInterop, thank you @marcobiedermann. I wish there were tests in this project like i18next or react-i18next, but the original committer did not add any. They are welcome if you want to work on them.

@jamuhl please merge and release should be patch

@marcobiedermann
Copy link
Contributor Author

marcobiedermann commented Aug 30, 2019

@rosskevin

Unfortunately I do not see any way how to write tests for this by now since this project is written in vanilla JavaScript and I would not add a test written in TypeScript to be honest. If you really want to tests this, along with all other TypeScript definitions, I suggest to port this project over to TypeScript entirely.

Would be great if you guys could merge this bug and release a fix on npm. Currently I forked this project and included it into my main project

@rosskevin
Copy link

@marcobiedermann if you want to see how to write usage tests, check out how we do it in i18next https://github.com/i18next/i18next/blob/master/package.json#L84

@jamuhl doesn't want to use typescript directly. I do think it would be much simpler for all of this source to be in typescript, but it's jamuhl's prerogative and I respect that.

@marcobiedermann
Copy link
Contributor Author

@rosskevin

Thank you very much for pointing out.
Yes a quick test would be to run the TypeScript compiler without outputting any code.
I did some quick testing and it seems like your TypeScript definitions do not match the current version of i18next.
In general, all your dependencies are quite outdated.
I suggest to update all to the most recent version and add TypeScript checking afterwards. Otherwise you run into too many compatibility issues.

@rosskevin
Copy link

Please update at least the minimum necessary i18next version in the package.json. Feel free to use ncu and update all.

PS I'm neither maintainer or user of this package, just helping out. I help maintain i18next ts primarily.

@jamuhl jamuhl merged commit 788ee1b into i18next:master Sep 2, 2019
@jamuhl
Copy link
Member

jamuhl commented Sep 2, 2019

published in i18next-express-middleware@1.8.2

@marcobiedermann marcobiedermann deleted the hotfix/typescript-import branch September 2, 2019 07:16
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