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

chore: output declaration files for neuroglancer packages #576

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

chrisj
Copy link
Contributor

@chrisj chrisj commented Apr 15, 2024

esbuild does not support generating declaration files evanw/esbuild#95

Add --declaration flag to build-package.ts to optionally output typescript declaration files
Enable flag for npm prepare script
@chrisj chrisj changed the title chore: added --declaration flag to build-package.ts to optionally output typ… chore: output declaration files for neuroglancer when installed with NPM Apr 15, 2024
outbase: srcDir,
bundle: false,
outdir: libDir,
});

if (declaration) {
buildDeclarationFiles(entryPoints, {
allowJs: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to remove these superfluous arguments

@chrisj chrisj force-pushed the cj_declaration_files branch from 7570bfc to 6724087 Compare April 15, 2024 16:27
@chrisj
Copy link
Contributor Author

chrisj commented Apr 15, 2024

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
Copy link
Collaborator

jbms commented Apr 15, 2024

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
Copy link
Collaborator

jbms commented Apr 15, 2024

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
Copy link
Contributor Author

chrisj commented Apr 15, 2024

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 chrisj changed the title chore: output declaration files for neuroglancer when installed with NPM chore: output declaration files for neuroglancer packages Apr 15, 2024
@chrisj
Copy link
Contributor Author

chrisj commented Apr 15, 2024

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
Copy link
Contributor Author

chrisj commented Apr 15, 2024

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.

@jbms jbms merged commit f1fd3b6 into google:master Jun 11, 2024
16 of 19 checks passed
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.

2 participants