ts-ignore on import in selection-types.ts

[jordanhboxer]

This needs to be added otherwise I have to disable noImplicitAny in my tsconfig.json which can hide other errors.
This wouldn't be needed either if the file were a compiled declaration file (.d.ts) because of skipLibCheck in the tsconfig.json so this seems like the simplest solution

[kwonoh]

I get the below error from a typescript project with "noImplicitAny": true while this repo has "noImplicitAny": false.

[TypeScript] Could not find a declaration file for module '../MosaicClient.js'. '<path>/@uwdata/mosaic-core/src/MosaicClient.js' implicitly has an 'any' type.
<path>/@uwdata/mosaic-core/src/util/selection-types.ts:2:30
    1 | import { SQLExpression } from '@uwdata/mosaic-sql';
  > 2 | import { MosaicClient } from '../MosaicClient.js';
      |                              ^^^^^^^^^^^^^^^^^^^^
    3 |
    4 | /**
    5 |  * Selection clause metadata to guide possible query optimizations.

Adding // @ts-ignore resolve the above issue.

However, if selection-types.ts will only contain type definitions, I think it should be named as selection-types.d.ts so the library users can use skipLibCheck for addressing this kind of issue.

[domoritz]

Can you enable skipLibCheck in your config? Wouldn't that ignore type errors in the library?

If we want to make a change here, I would suggest that we enable noImplicitAny to check everything and avoid potential regressions and instead of ts-ignore, should we add the missing declarations?

[kwonoh]

skipLibCheck only skips *.d.ts files but selection-types.ts is a *.ts file not a *.d.ts file. so skipLibCheck doesn't skip selection-types.ts

We weren't sure about the scope of selection-types.ts whether it will only contain type definitions or actual functioning code. If it will only contain type definitions, selection-types.ts should be renamed to selection-types.d.ts.

I think changing "noImplicitAny": false to "noImplicitAny": true is a big change for mosaic that will essentially require actual typescript support from mosaic.

[domoritz]

I see. I'm okay with moving to .d.ts files for everything but iirc there was some reason @jheer used ts files.

Id love to have a setup that lets is gradually transition to ts. If you could make a proposal that would be awesome.

Happy to merge this pull request if it's a stopgap but you would probably need a release, no?

[kwonoh]

Yes, we would need a release. But we can wait for @jheer's response on whether changing selection-types.ts to selection-types.d.ts will work.

[domoritz]

If you can, go ahead and try.

[kwonoh]

Id love to have a setup that lets is gradually transition to ts. If you could make a proposal that would be awesome.

Yes, we'd like to contribute TS support. Is there a plan on a direction for TS support? like adding type definitions (*.d.ts files) along with the current JS code OR porting the current JS code to TS?

[kwonoh]

Changing selection-types.ts to selection-types.d.ts seems to work locally. I didn't see any errors from build/lint/test/docs:build. Waiting for the repo's workflow to kick in.

[domoritz]

Support supportive of going ts but we want to make sure it's done in a way where we don't need to re-compile a sub-project to get the updates in another. Ideally, we figure out some way to do gradual transitions, that would be best and then send follow up PRs to change individual models.

Related: #312

[jheer]

I need to understand the issue better. Right now the setup is to primarily use JavaScript (.js), alongside TypeScript (.ts) files to more conveniently define types.

I think what's missing (relative to what I've done in other projects like Arquero and Flechette), is then use both of these as input to tsc as part of the build process to produce "official" types as .d.ts files that are linked from the types property in package.json.

So why don't we do that instead, and see if it resolves import issues?

[jordanhboxer]

@jheer That sounds like the ideal solution given that that will produce the correct output in the built package rather than using a declaration file in the source code.

To give you some more context, our build pipeline runs tsc on all .ts files and since it hasn't been compiled to a declaration file we can't ignore it skipLibCheck. Setting "noImplicitAny": false is not ideal for us either as that will hide potential issues.

[domoritz]

Like this #545?

[jordanhboxer]

I believe so!

[domoritz]

Should be fixed by #545.