google / neuroglancer

WebGL-based viewer for volumetric data
Apache License 2.0
1.02k stars 283 forks source link

chore: output declaration files for neuroglancer packages #576

Closed chrisj closed 3 weeks ago

chrisj commented 2 months ago

esbuild does not support generating declaration files https://github.com/evanw/esbuild/issues/95

chrisj commented 2 months ago

I improved the quality of the declarations by using the tsconfig file. The bulk of the change comes from "moduleResolution": "bundler",

It does lose a little bit of type information for some reason but it adds a lot more than it removes

-    spatiallyIndexedSources: Set<Borrowed<AnnotationGeometryChunkSource>>;
+    spatiallyIndexedSources: Set<AnnotationGeometryChunkSource>;
-    references: Map<AnnotationId, Borrowed<AnnotationReference>>;
-    localUpdates: Map<AnnotationId, LocalUpdateUndoState>;
+    references: Map<string, AnnotationReference>;
+    localUpdates: Map<string, LocalUpdateUndoState>;
jbms commented 2 months ago

Thanks --- we could probably just emit the declarations all the time --- I don't think there is a time when we don't want the declarations.

jbms commented 2 months ago

Having the type aliases resolved in some cases is fine, I think, and I agree that some annotations are better than none.

When I tried to generate declarations previously I ran into some errors due to some exported things having types depending on non-exported types, I think. But I guess it works now for some reason.

chrisj commented 2 months ago

Removed the flag making it optional. Added that in case the performance benefit of leaving it off was useful.

I agree imperfect types are far better than none. I was fortunate that this solution https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#getting-the-dts-from-a-javascript-file was very easy to drop in, and worked

chrisj commented 2 months ago

I just realized const host = ts.createCompilerHost(options); is probably unnecessary but it does change the declaration files. Removes ".ts" from the imports

Checking to make sure that isn't a problem. Update: All good

chrisj commented 2 months ago

check(new Uint64(1423829346, 3183191017), 28); in toString parseString round tripis a real error. I'm trying to debug it

check(new Uint64(0, 79741775), 29); also fails, and it seems to fail with any low value.

79741775 seems to be the smallest high that causes this test to fail with base 29. It is also the first high value that causes low to be less than trueBase, as a result of highRemainder being very small.