molszanski / iti

~1kB Dependency Injection Library for Typescript and React with a unique support of async flow
https://itijs.org
MIT License
129 stars 6 forks source link

Type resolution fails when importing iti with "moduleResolution": "NodeNext" #37

Open tmillican opened 1 year ago

tmillican commented 1 year ago

Problem Description

When using iti in a TypeScript project with "moduleResolution": "NodeNext" in your tsconfig.json, type information for iti cannot be located.

This line:

import { createContainer } from 'iti'

Produces the error:

Could not find a declaration file for module 'iti'. '<project path>/node_modules/iti/dist/iti.modern.js'
implicitly has an 'any' type.
  Try `npm i --save-dev @types/iti` if it exists or add a new declaration (.d.ts) file
  containing `declare module 'iti';`

The project will still compile and run with tsc; you just don't have type information for iti for static checking, linting, etc.

Cause

There are two causes:

  1. The exports map in iti's package.json contains an exports map, but the types key is not defined for each export. See this TypeScript issue for a complete explanation. When exports is defined, top-level types or typings keys are ignored.

  2. The export ... from statements in iti/src/index.ts need to use an explicit .js extension. Without this, import { createContainer } from 'iti' will nominally succeed, but the type of createContainer will just be import, and the static analysis gets very confused.

Proposed fix

  1. The first issue is simple: just add "types": "./dist/src/index.d.ts" to the exports map in package.json.

  2. For the second issue, I'll be honest: I'm fairly new to JS/TS. My understanding is that explicit .js extensions are required for import/export statements when using ESM ("type": "module" in package.json). So I'm not sure how iti is getting away with "bare" imports in the first place. But in any case, adding extensions only to the export statements in src/index.ts is enough to satisfy the consumer's linter and should be backwards-compatible with CJS. With that said, two of the iti tests (dispose....) try to import { createContainer } from '../src', which needs to be ../src/iti after this change. The other tests already import this way.

I have a fork with these proposed changes made to iti and iti-react. I can make a PR if you'd like.

molszanski commented 1 year ago

Oh great! Thank you @tmillican for opening this up! Will check it and merge into the upstream 🙇‍♂️

molszanski commented 1 year ago

Also, this is a great description, I've learned something new :)

danielmahon commented 1 year ago

@molszanski thoughts on getting this fix added?

molszanski commented 1 year ago

@danielmahon oh! Yeah, forgot about it! Will release a minor with better cjs/mjs compatibility!