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

Export class Viz #224

Closed
wants to merge 3 commits into from
Closed

Export class Viz #224

wants to merge 3 commits into from

Conversation

mfbx9da4
Copy link

No description provided.

@mdaines
Copy link
Owner

mdaines commented Dec 27, 2023

Can you provide an explanation of this change? What does adding the export keyword accomplish?

Also, does this change the results of the type tests? https://github.com/mdaines/viz-js/blob/3ef0b7f41d315d52048e6c6246d054b62b8d3017/packages/viz/test/types/top-level.ts

The Viz class is not exported in JavaScript, so the desired effect of the declaration file is to provide the type information but not declare that the class itself is available directly.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Dec 28, 2023

Hi I was passing around the Viz instance eg

function visualize(viz: Viz) {
  // ...
}

Since it wasn't exported I couldn't reference the type. I think it's obvious enough to the user they can't construct the Viz class themselves. You could make it even clearer by adding the following:

class Viz {
  /** Cannot construct `Viz` class directly use `instance()` instead */
  private constructor() {
    throw new Error("Cannot construct `Viz` class directly use `instance()` instead");
  }
}

That way new Viz() will always throw a typescript error and tell them what to do instead.

Regarding the tests, yes, likely those tests will now fail.

@mdaines
Copy link
Owner

mdaines commented Dec 28, 2023

That seems like it should work, but I don't want to just hide the constructor and leave the class in the declaration because that doesn't match what happens in JavaScript. I do want to be able to reference Viz for typechecking, though.

It looks like this can be done with the type modifier, but I'll need to add a type export. See #225.

import { instance, type Viz } from "@viz-js/viz";

function myRender(viz: Viz, src: string) {
  return viz.render(src);
}

instance().then(viz => {
  myRender(viz, "digraph { a -> b }");
});

I'm also going to enable the verbatimModuleSyntax flag in the type tests.

@mdaines
Copy link
Owner

mdaines commented Jan 5, 2024

I'm closing this in favor of #225. Thank you for pointing this out.

@mdaines mdaines closed this Jan 5, 2024
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