allen-cell-animated / volume-viewer

https://allen-cell-animated.github.io/volume-viewer/
Other
90 stars 7 forks source link

Fully qualified imports #190

Closed frasercl closed 4 months ago

frasercl commented 4 months ago

Review time: pretty quick (15min)

186 added "type": "module" to package.json. Unknown to us, doing this caused this package to come under stricter module resolution rules. Specifically, paths to modules must be fully specified: no leaving off the .js extension in a file path, and no expecting the resolver to implicitly find an index.js if the path is a directory. So while the test app still works just fine on main, the package breaks when used by any downstream packages (website-3d-cell-viewer).

The rules and (conflicting) standards around how the JS module system should work are... complicated... but TypeScript's current guidance seems to be that modules should be referenced by the fully qualified path they will have after they are built, and TypeScript will handle resolving back from js-land to ts-land when type checking:

There is no compiler option that enables transforming, substituting, or rewriting module specifiers. Consequently, module specifiers must be written in a way that works for the code’s target runtime or bundler, and it’s TypeScript’s job to understand those output-relative specifiers.

So this PR fixes the issue by fully specifying all imports across this project, as TypeScript tells us to do. It also adds a bunch of type keywords to imports to help TypeScript know to elide them.

Steps to verify:

Then, for whichever branch you want to test:

Expected behavior on main: lots of errors.

Expected behavior on this branch (fix/fully-qualified-imports): works just fine.

toloudis commented 4 months ago

npm start doesn't work for me on this volume-viewer branch, yet

I think the test viewer is importing from src/ instead of the compiled es/ directory. This is something we ran into in simularium-viewer too and we had to make a choice of which version of the code the test viewer should be loading. In one case you don't have to rebuild but in the other you do.

My preference is slightly toward using es/ because it better emulates what real clients of this package would do, but it DOES slow down debugging if you have to recompile over and over. Would be cool if we could do both.