Closed jasonbosco closed 2 years ago
@dcantu476 @AadamZ5
Any thoughts on this? The .ts
src files are currently not published to npm, which I'm guessing is why these warnings show up. Is it standard practice to publish the src
directory to npm for Typescript packages?
I'll take a peek at it when I can, but an extra set of eyes is welcome as I'm unfamiliar with parcel. It appears it's trying to package source maps as part of the build pipeline.
Professionally speaking, you should only publish artifacts such as the dist or lib folder with the .d.ts files (type definitions) and compiled .js files, not src files. On a practical level, though, you'll find plenty of packages with src folders published.
EDIT: One last thought I had after posting this. It's possible we are publishing development builds when we should be publishing production builds to the npm repo. I'd have to look into how we are building the library, but there shouldn't be any reference to source maps in the package at all.
EDIT: Also disclaimer, I am not familiar with Parcel either. However, I don't think this issue is particular to it.
I think @dcantu476 is definitely right. Looks like this library is publishing *.js.map
files, which tell compilers/interpreters where to find source code. In a development build, this is useful. However, for production and distribution, these *.js.map
files shouldn't be included since no source code is distributed with it. End users should not be debugging this library inside of their own projects.
You can see inside lib/Typesense.js.map
what it contians
{
"version":3,
"file":"Typesense.js",
"sourceRoot":"",
"sources":["../src/Typesense.ts"],
"names":[],
"mappings":"...<omitted>..."
}
Any compiler/interpreter/debugger/whatever reading this file will look for the path(s) at the "sources"
key, which is ["../src/Typesense.ts"]
, which doesn't exist in the released and published version.
I think for release, this library needs to be configured with different options so that the TS compiler doesn't generate *.js.map
files. (*.d.ts
files should stay). This could be done by extending TS config files. During a release build, point the tsc
compiler to the release TS configuration file, that extends and overrides the default/development config file.
A production/release TS config file (maybe named "tsconfig.prod.json") might look like this:
{
"extends": "./tsconfig",
"compilerOptions": {
"sourceMap": false
}
}
Then you'd have to build using this new config before publishing a new version to the package repository. Be careful, I don't think that tsc
cleans/deletes the existing lib
folder.
Hope that explanation wasn't confusing 😅
Thank you for the detailed explanation Damian and Aadam!
IIRC, I might have explicitly turned on source maps several years ago because I found it helpful to debug and get to the bottom of bugs quickly, when I used the library myself in other projects. The source maps are generated as separate files though, so they don't end up getting including in users' production builds (or at least that was my understanding of how it works). Is it not standard for libraries to bundle source maps (as separate files) in NPM packages this way?
If not, if users of the library wanted to report bugs they see, wouldn't it be hard to get stack traces without source maps?
Found this interesting discussion: https://github.com/googleapis/google-cloud-node/issues/2867
I'm now wondering if we should just publish the .ts
files in the src
folder as well.
I've added the src folder for now.
EDIT: Never mind it was my mistake. I made a bad import
import { CollectionCreateSchema } from 'typesense/src/Typesense/Collections';
whereas it should be
import { CollectionCreateSchema } from 'typesense/lib/Typesense/Collections';
I leave my senseless blabber below. I don't know how I managed to include it from source. I guess it wasn't intended for these types to be used outside of the package, but I really want my project to be strongly typed where it matters, but then I have to stumble upon foot guns like these.
It can be nice that the source is included with source maps, no doubt, but I don't think this was meant to be done, and so Typescript was designed so that you only publish .d.ts
files and .js
/.mjs
/.cjs
files in your package. I am getting tsc
errors now when I want to use "strict": true
:
node_modules/typesense/src/Typesense/ApiCall.ts:59:7 - error TS2322: Type 'null' is not assignable to type 'ResponseType'.
59 responseType = null
~~~~~~~~~~~~
node_modules/typesense/src/Typesense/ApiCall.ts:94:7 - error TS2322: Type 'null' is not assignable to type 'ResponseType'.
94 responseType = null
~~~~~~~~~~~~
node_modules/typesense/src/Typesense/ApiCall.ts:206:86 - error TS2571: Object is of type 'unknown'.
206 `Request #${requestNumber}: Request to Node ${node.index} failed due to "${error.code} ${error.message}${
~~~~~
...
Or maybe I just didn't set up my tsconfig.json
properly?
{
"compilerOptions": {
"strict": true,
"module": "commonjs",
"moduleResolution": "Node",
"lib": ["es2021"],
"declaration": true,
"removeComments": true,
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"allowSyntheticDefaultImports": true,
"target": "es2017",
"sourceMap": true,
"outDir": "./dist",
"baseUrl": "./",
"incremental": true,
"skipLibCheck": true
},
"exclude": ["./node_modules"]
}
I really don't know what other option I can add to ignore .ts
files under node_modules
. Anyways it currently seems to me now that this package, because it publishes src
, forbids me to use strict mode ☹️.
The presence of the src
dir in the published package also causes issue for me too when using strict mode.
../../node_modules/typesense/src/Typesense/ApiCall.ts:343:25 - error TS7006: Parameter 'node' implicitly has an 'any' type.
343 nodeDueForHealthcheck(node, requestNumber = 0): boolean {
~~~~
../../node_modules/typesense/src/Typesense/ApiCall.ts:367:22 - error TS7006: Parameter 'node' implicitly has an 'any' type.
367 setNodeHealthcheck(node, isHealthy): void {
~~~~
etc.....
I'm not sure why these src/...
files are being picked up by TypeScript, but I can only get things to compile by turning off strict mode (undesirable).
Description
Running
parcel build
shows these warnings:Steps to reproduce
yarn install
parcel build
Expected Behavior
No warnings are logged
Actual Behavior
The warnings above are logged
Metadata
Typsense.js Version: 1.0.3-2