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

Add Typed Files/Groups #34

Open
Carnageous opened this issue Sep 18, 2022 · 5 comments
Open

Add Typed Files/Groups #34

Carnageous opened this issue Sep 18, 2022 · 5 comments

Comments

@Carnageous
Copy link

I want to contribute a feature I will likely need in the future: Adding types to Files/Groups in order to describe their content. For example, if an HDF5 file has some groups like "data" and "metadata", each containing some datasets, an interface for the File could look like this:

interface MyHDF5File {
  data:  {
    dataset_1: Dataset;
    subsets: {
      dataset_3: Dataset;
    }
  },
  metadata: {
    metadata_1: Dataset;
  }
}

With this, you could "strongly type" any file, and even get type hints in group.get() methods. (with typescript Template Literals)

const file = h5wasm.File<MyHDF5File>(...)

file.get("/data/dataset_4") // results in typescript error, as it does not exist in the Interface.

First of all, would it okay to add such a feature? If so, I would like to implement it and open a PR. It would, of course, be optional and should not break any existing code.

While working with h5wasm locally, I noticed some things:

  1. Some tests seem to be failing. For datatype_test.mjs I get an error:
AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ Datatype {
- {
    type: 'Datatype'
  }
  1. There is no proper description in the readme on how to build h5wasm. It's quite straight-forward, but adding a small step-by-step guide could be helpful (also naming the emscripten SDK as a dependency). If you want I can open a PR for that, though I am not very familiar with the emscripten/c ecosystem.

As a side-note, I saw in the git history that you thought about switching to esbuild for building h5wasm? Is there any help needed there?

Thank you!

@aLemonFox
Copy link

I like the TypeScript idea! Currently I need to cast all my groups/datasets like this:

const attrs = group.attrs as unkown as MyAttrs;

@axelboc
Copy link
Collaborator

axelboc commented Sep 19, 2022

Love the idea of typing files more strongly!

Just wanted to point out that the syntax below is a type assertion in disguise:

const file = h5wasm.File<MyHDF5File>(...)

This ESLint community rule argues against using this pattern. Perhaps it would be better to keep an explicit type cast?

@bmaranville
Copy link
Member

Yes, please feel free to make a PR for typed Files/Groups.

Also, to answer your other points above:

  1. I think the test is fixed
  2. There is a DEVELOPER.md but it is not linked from README.md - and I need to add instructions for the typescript build in addition to the emscripten build.
  3. I did have esbuild working for a while but I switched to tsc because I wanted strict typescript compliance to be enforced as a side-effect of building instead of adding it as a separate testing step. If there is a better system for accomplishing this I am open to PRs on this as well.

@Carnageous
Copy link
Author

const file = h5wasm.File<MyHDF5File>(...)

This ESLint community rule argues against using this pattern. Perhaps it would be better to keep an explicit type cast?

Thank you a lot for the feedback @axelboc! Like you said it is a type assertion, and I was not aware this is discouraged.
Could you point out a syntax you would prefer? Based on the ESLint rule, something like this maybe:

const file: File<MyType> = h5wasm.File(...)
// or
const file = h5wasm.File(...) as File<MyFile>

@axelboc
Copy link
Collaborator

axelboc commented Sep 19, 2022

Basically, h5wasm.File() should return unknown, which (if I'm not mistaken) would force the use of the as syntax in strict mode. Though consumers would also be free to write a proper type guard/assertion:

function isMyFile(file: unknown): file is File<MyFile> {
 return /* some condition that is sufficient to say that file is of type `File<MyFile>` */;
}

const file = h5wasm.File(..);
if (isMyFile(file)) {
  // file has type // File<MyFile>
}

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

4 participants