zemlyansky / modelzoo

TensorFlow.js models
https://www.npmjs.com/package/modelzoo
0 stars 0 forks source link

Missing exporting types for npm package #1

Open peacefulotter opened 9 months ago

peacefulotter commented 9 months ago

Types are not included on npm which results in the following:

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

Could you include them as well please? Thanks!

zemlyansky commented 9 months ago

Hi, @peacefulotter, do you see this error when trying to import the module in typescript project? Shouldn't it face src/index.ts then?

peacefulotter commented 9 months ago

Yes, it is in a typescript project. I think node looks at the dist folder when importing packages and the tsconfig.json is not configured properly to export types - it only exports js. Also, installing the package shouldnt include the entire repo but just what the user needs. Again, its a tsconfig.json / package.json issue. As we have talked about, feel free to take inspiration for here: https://github.com/peacefulotter/react-leaflet-hotline It should still be a good template for creating packages in cjs/esm!

zemlyansky commented 9 months ago

@peacefulotter, i'm probably missing something. Why would you need additional type export if you are importing a typescript project into another typescript project? It seems that what's missing is "types": "src/index.ts" in package.json? 7 config files seems like overkill at this stage

zemlyansky commented 9 months ago

Also, installing the package shouldnt include the entire repo but just what the user needs. Again, its a tsconfig.json / package.json issue.

@peacefulotter, could you explain this point? how do you know what users need when doing npm install? or do you mean treeshaking doesn't work?

upd. Thanks for opening the issue and generally pushing towards using TS :)

zemlyansky commented 9 months ago

Added "types": "./src/index.ts" in a83e143 and pushed to npm

zemlyansky commented 9 months ago

Just made a small experiment: two packages a and b with own package.json files and src, dist folders. a defines sum in a.ts as:

export function sum (a: number, b: number): number {
  return a + b 
}

b imports a in b.ts:

import * as a from '../../a'
console.log(a.sum(1, 2))
console.log(a.sum('x', 'y'))

Setting "types": "./src/a.ts" in package.json of a makes b throw TS2345 when trying to compile console.log(a.sum('x', 'y')) (as expected).

src/b.ts:4:19 - error TS2345: Argument of type 'string' is not assignable to parameter of type 'number'.

Removing "types": "./src/a.ts" shows warning in VSCode and TS7016 error in strict mode:

Could not find a declaration file for module '../../a' 

It seems that pointing "types" to the original ts code works. The question is still valid: why using additional .d.ts instead?

peacefulotter commented 9 months ago

@peacefulotter, i'm probably missing something. Why would you need additional type export if you are importing a typescript project into another typescript project? It seems that what's missing is "types": "src/index.ts" in package.json? 7 config files seems like overkill at this stage

The package loads files from the dist folder, which are in JS. Since you don't include the types in the dist folder, it can't find them. I find it practical to have 3 tsconfig.json, one for esm, one for cjs and one shared between the two that defines common settings.

peacefulotter commented 9 months ago

Just made a small experiment: two packages a and b with own package.json files and src, dist folders. a defines sum in a.ts as:

export function sum (a: number, b: number): number {
  return a + b 
}

b imports a in b.ts:

import * as a from '../../a'
console.log(a.sum(1, 2))
console.log(a.sum('x', 'y'))

Setting "types": "./src/a.ts" in package.json of a makes b throw TS2345 when trying to compile console.log(a.sum('x', 'y')) (as expected).

src/b.ts:4:19 - error TS2345: Argument of type 'string' is not assignable to parameter of type 'number'.

Removing "types": "./src/a.ts" shows warning in VSCode and TS7016 error in strict mode:

Could not find a declaration file for module '../../a' 

It seems that pointing "types" to the original ts code works. The question is still valid: why using additional .d.ts instead?

.d.ts files are used along with .js files to provide types when building to javascript files. This way your package is compatible with both languages at the same time. What I did in react-leaflet-hotline is similar to what you are doing: build into esm and cjs folders separately but I have an additional folder types containing one .d.ts for each built js file.

zemlyansky commented 9 months ago

@peacefulotter thanks! i was trying to figure out what are the benefits of using compiled js + types in .d.ts comparing to loading raw .ts files from src in case of typescript. it seems that you can do both ways. one case when .d.ts are really required is when original typescript implementation is hidden and then you import js with extra types, but in this case what problem they solve? why not import ts?

The package loads files from the dist folder

That's fixed with "types": "src/index.ts" in package.json defining where typescript imports should look at.

just to be clear, not against this - quite simple to add :) still learning

peacefulotter commented 9 months ago

@peacefulotter thanks! i was trying to figure out what are the benefits of using compiled js + types in .d.ts comparing to loading raw .ts files from src in case of typescript. it seems that you can do both ways. one case when .d.ts are really required is when original typescript implementation is hidden and then you import js with extra types, but in this case what problem they solve? why not import ts?

The package loads files from the dist folder

That's fixed with "types": "src/index.ts" in package.json defining where typescript imports should look at.

just to be clear, not against this - quite simple to add :) still learning

I mean, again, maybe it works - but it is definitely not very scalable and I don't really think this is the typical way to go. Although, there might be some other reasons why you would want or don't want to do this, I am not an expert. Best