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

TypeScript compilation fails #272

Closed
here-abarany opened this issue Apr 20, 2023 · 5 comments
Closed

TypeScript compilation fails #272

here-abarany opened this issue Apr 20, 2023 · 5 comments

Comments

@here-abarany
Copy link

here-abarany commented Apr 20, 2023

As of about two weeks ago, TypeScript compilation fails with the following error:

/usr/local/lib/node_modules/ts-node/src/index.ts:859
    return new TSError(diagnosticText, diagnosticCodes, diagnostics);
           ^
TSError: ⨯ Unable to compile TypeScript:
3d-tiles-validator/src/packages/TilesetPackage3dtiles.ts(42,45): error TS18046: 'row' is of type 'unknown'.
3d-tiles-validator/src/packages/TilesetPackage3dtiles.ts(52,14): error TS18046: 'row' is of type 'unknown'.
    at createTSError (/usr/local/lib/node_modules/ts-node/src/index.ts:859:12)
    at reportTSError (/usr/local/lib/node_modules/ts-node/src/index.ts:863:19)
    at getOutput (/usr/local/lib/node_modules/ts-node/src/index.ts:1077:36)
    at Object.compile (/usr/local/lib/node_modules/ts-node/src/index.ts:1433:41)
    at Module.m._compile (/usr/local/lib/node_modules/ts-node/src/index.ts:1617:30)
    at Module._extensions..js (node:internal/modules/cjs/loader:1329:10)
    at Object.require.extensions.<computed> [as .ts] (/usr/local/lib/node_modules/ts-node/src/index.ts:1621:12)
    at Module.load (node:internal/modules/cjs/loader:1133:32)
    at Function.Module._load (node:internal/modules/cjs/loader:972:12)
    at Module.require (node:internal/modules/cjs/loader:1157:19) {
  diagnosticCodes: [ 18046, 18046 ]
}

This seems to correspond with 5.0.3 release of TypeScript 2 weeks ago. I tried manually installing typescript 4.9.5, and verified that both the 3d-tiles-validator and global node_modules have that version installed, but it still fails.

For some reason it works locally on Arch Linux with Node 19, even after deleting the cached node modules. However, it fails both on Rocky Linux 9 with Node 16 and Alpine Linux with Node 19 when running through Docker.

This affects the tagged versions (0.4.0), and HEAD as at least before the switch to use the external 3d-tiles-tools. (I haven't been able to verify since the switch to external 3d-tiles-tools due to CesiumGS/3d-tiles-tools#23)

@javagl
Copy link
Contributor

javagl commented Apr 21, 2023

I had seen this error in the CI build at #271 , but this PR had become obsolete in the meantime, due to the switch to the external 3d-tiles-tools.

I'd expect the same error to appear in the 3d-tiles-tools now. The CI for the 3d-tiles-tools is not set up yet, but that's already tracked as CesiumGS/3d-tiles-tools#19 and one of the next points on my TODO list.

I have not yet investigated the reason for this error, though. I assume that it is just a trivial thing, probably just some /* typescript-ignore-this */ or undefined-check (or row as any) to be slapped in at the right place. But since I only once saw the error, and could not yet reproduce it locally, I'll have to investigate that first. If you have any ideas, just drop me a note.

@javagl
Copy link
Contributor

javagl commented Apr 23, 2023

@here-abarany You mentioned

This seems to correspond with 5.0.3 release of TypeScript 2 weeks ago.

I don't see why this TypeScript version should be relevant here. It should not use a 5.x.x version anyhow.

I investigated this a bit, based on the (closed and obsolete) PR and branch mentioned above, I'm about 99.9% sure that this issue is caused by DefinitelyTyped/DefinitelyTyped#65035

  • The package.json contained "@types/better-sqlite3": "^7.6.2",
  • Apparently, the changes in the types went into version 7.6.3 (sooo.... that's not semantic versioning, apparently)
  • Pinning to 7.6.2 makes it work.

The sequence of commits is at https://github.com/javagl/3d-tiles-validator/commits/unzip-package-entries

The last one contains a workaround for the issue. Basically two as any at the right place. I see the intention behind using unknown and not using any: One cannot be sure that the row contains the required key/content properties! One could check the type of the row each time, roughly as in

if (typeof row === 'object' && row !== null && 'key' in row && 'content' in row) {
    // Yup, all good...
}

But ... that looks quirky to me: Whether or not a row matches this criterion depends on the table structure. Sure, that's not "compile-time-safe", but ... that's how databases are. So to address this, I added a function validateTableStructure that checks whether the table (and therefore, each row) has the required structure.

That could/should have been done anyhow, particularly in the validator. Right now, the TilesetPackage3dtiles basically had zero error handling, and silently assumed that the table has the right structure. I'll probably extend this (and maybe add some tests) before moving this code block and workaround into the 3d-tiles-tools.

@here-abarany
Copy link
Author

@javagl I thought it might have been the typescript release and somehow not respecting the pinned version based on the timing of the release, but your results of looking at the better-sqlite3 types release makes more sense.

Thanks for getting the fix in. Do you have a rough idea of when a 3d-tiles-tools release will be made and 3d-tiles-validator will be able to take advantage of both that fix and the base better-sqlite3 package that's compatible with newer versions of Node?

@javagl
Copy link
Contributor

javagl commented Apr 25, 2023

The validator and tools have largely been rewritten, and there will probably be some glitches like that in the first releases, but we're trying to converge to a stable state here. (Discalimer: As stable as it can be, with things like that @types patch version change that caused the compilation to fail...)

The current (main) state of the 3d-tiles-tools is already a candidate for a 0.2.1 release. Further testing would be good, and feedback is always appreciated. We're planning to do a new release for that ~"in the next few days" (end of this week, or early next week). A new release of the 3d-tiles-validator will follow shortly after that, using that 3d-tiles-tools release as a dependency, and probably omitting the better-sqlite3 dependency (that should now be unnecessary there).

@javagl
Copy link
Contributor

javagl commented Apr 25, 2023

(BTW: I think this can be closed, but if there is any other feedback, feel free to add it here, or open a new issue)

@javagl javagl closed this as completed Apr 25, 2023
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

No branches or pull requests

2 participants