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

Error in TypeScript compilation: Cannot find name 'StdioOverrideFunction' #40

Closed
bvobart opened this issue Apr 8, 2024 · 8 comments · Fixed by #41
Closed

Error in TypeScript compilation: Cannot find name 'StdioOverrideFunction' #40

bvobart opened this issue Apr 8, 2024 · 8 comments · Fixed by #41

Comments

@bvobart
Copy link

bvobart commented Apr 8, 2024

Hi, I'm trying to use your library in a project for work, but running npm run build in our project fails due to the following error:

node_modules/wasmagic/dist/index.d.ts:63:13 - error TS2304: Cannot find name 'StdioOverrideFunction'.

63     stdio?: StdioOverrideFunction;
               ~~~~~~~~~~~~~~~~~~~~~


Found 1 error in node_modules/wasmagic/dist/index.d.ts:63

I see that the mentioned StdioOverrideFunction is defined in the types folder in this repo, but not in src/index.ts and it's apparent that the TypeScript compiler isn't including that type into the final dist/index.d.ts.

Can you fix this? It's breaking TypeScript builds at the moment.

@bvobart
Copy link
Author

bvobart commented Apr 8, 2024

Note: I created a patch file with patch-package to work around the issue for now. Here's the patch file:

diff --git a/node_modules/wasmagic/dist/index.d.ts b/node_modules/wasmagic/dist/index.d.ts
index a4bc2f5..c59e913 100644
--- a/node_modules/wasmagic/dist/index.d.ts
+++ b/node_modules/wasmagic/dist/index.d.ts
@@ -56,6 +56,10 @@ export declare enum WASMagicFlags {
     /** Don't check for SIMH tape files */
     NO_CHECK_SIMH = 8388608
 }
+export type StdioOverrideFunction = (
+    stdioName: "stdout" | "stderr",
+    text: string,
+) => void;
 export type WASMagicOptions = {
     flags?: WASMagicFlags;
     loadDefaultMagicfile?: boolean;

@moshen
Copy link
Owner

moshen commented Apr 14, 2024

Thanks! Sorry about that. I'll get this integrated. I haven't run into this when testing myself

moshen added a commit that referenced this issue Apr 15, 2024
This was a case of bad management of the types in play and their
exposure outside the package. The fix that works for both package
maintenance, and external users is to ensure that the type is available
and copied into the final npm package. Finally this would have been
caught with a test, which has now been added.

- Fix #40
- Add dedicated type file for StdioOverrideFunction
- Change type file paths to be relative
- Move all type files to the `src` folder to make things easier
- Add Makefile rules to copy over `.d.ts` files so they are in the
  package
moshen added a commit that referenced this issue Apr 15, 2024
This was a case of bad management of the types in play and their
exposure outside the package. The fix that works for both package
maintenance, and external users is to ensure that the type is available
and copied into the final npm package. Finally this would have been
caught with a test, which has now been added.

- Fix #40
- Add dedicated type file for StdioOverrideFunction
- Change type file paths to be relative
- Move all type files to the `src` folder to make things easier
- Add Makefile rules to copy over `.d.ts` files so they are in the
  package
moshen added a commit that referenced this issue Apr 15, 2024
This was a case of bad management of the types in play and their
exposure outside the package. The fix that works for both package
maintenance, and external users is to ensure that the type is available
and copied into the final npm package. Finally this would have been
caught with a test, which has now been added.

- Fix #40
- Add dedicated type file for StdioOverrideFunction
- Change type file paths to be relative
- Move all type files to the `src` folder to make things easier
- Add Makefile rules to copy over `.d.ts` files so they are in the
  package
moshen added a commit that referenced this issue Apr 15, 2024
This was a case of bad management of the types in play and their
exposure outside the package. The fix that works for both package
maintenance, and external users is to ensure that the type is available
and copied into the final npm package. Finally this would have been
caught with a test, which has now been added.

- Fix #40
- Add dedicated type file for StdioOverrideFunction
- Change type file paths to be relative
- Move all type files to the `src` folder to make things easier
- Add Makefile rules to copy over `.d.ts` files so they are in the
  package
moshen added a commit that referenced this issue Apr 15, 2024
This was a case of bad management of the types in play and their
exposure outside the package. The fix that works for both package
maintenance, and external users is to ensure that the type is available
and copied into the final npm package. Finally this would have been
caught with a test, which has now been added.

- Fix #40
- Add dedicated type file for StdioOverrideFunction
- Change type file paths to be relative
- Move all type files to the `src` folder to make things easier
- Add Makefile rules to copy over `.d.ts` files so they are in the
  package
@moshen
Copy link
Owner

moshen commented Apr 15, 2024

A little bit of fighting with the Typescript type system for the type to not only be exposed in the library, but to work in-editor for library maintenance. My original solution, and your patch @bvobart were flawed, in that local development of the library with type checking was broken.

As the StdioOverrideFunction hook is injected when the C wrapper is built with Emscripten, I had to move the type definition into it's own file and re-export it to use ergonomically in the library.

Thank you for the bug report! This should be fixed in the latest release. Please let me know if you have any other issues.

moshen added a commit that referenced this issue Apr 15, 2024
This was a case of bad management of the types in play and their
exposure outside the package. The fix that works for both package
maintenance, and external users is to ensure that the type is available
and copied into the final npm package. Finally this would have been
caught with a test, which has now been added.

- Fix #40
- Add dedicated type file for StdioOverrideFunction
- Change type file paths to be relative
- Move all type files to the `src` folder to make things easier
- Add Makefile rules to copy over `.d.ts` files so they are in the
  package
@bvobart
Copy link
Author

bvobart commented Apr 17, 2024

Thanks for your efforts!! Yeah, my patch was aimed at the final compiled & published artifacts, I don't know exactly how that relates back to your source code.

However, the issue is not yet fixed. It seems that your fix now doubly includes the StdioOverrideFunction: there's both an import StdioOverrideFunction from ..; export { default as StdioOverrideFunction } from .. as well as another declaration export type StdioOverrideFunction, causing the following errors when I try to compile my project with the latest version of wasmagic:

node_modules/wasmagic/dist/index.d.ts:1:8 - error TS2395: Individual declarations in merged declaration 'StdioOverrideFunction' must be all exported or all local.

1 import StdioOverrideFunction from "./StdioOverrideFunction";
         ~~~~~~~~~~~~~~~~~~~~~

node_modules/wasmagic/dist/index.d.ts:1:8 - error TS2440: Import declaration conflicts with local declaration of 'StdioOverrideFunction'.

1 import StdioOverrideFunction from "./StdioOverrideFunction";
         ~~~~~~~~~~~~~~~~~~~~~

node_modules/wasmagic/dist/index.d.ts:2:10 - error TS2484: Export declaration conflicts with exported declaration of 'StdioOverrideFunction'.

2 export { default as StdioOverrideFunction } from "./StdioOverrideFunction";
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/wasmagic/dist/index.d.ts:61:13 - error TS2395: Individual declarations in merged declaration 'StdioOverrideFunction' must be all exported or all local.

61 export type StdioOverrideFunction = (
               ~~~~~~~~~~~~~~~~~~~~~


Found 4 errors in the same file, starting at: node_modules/wasmagic/dist/index.d.ts:1

I'll fork and play around to see if I can fix it in your source code / build process. I'll report back when I have a PR, but of course feel free to fix it yourself in the mean time too :)

@moshen moshen reopened this Apr 17, 2024
@moshen
Copy link
Owner

moshen commented Apr 17, 2024

Do you have an example of usage / Typescript settings? I'm probably missing something here. When I import into another typescript project it compiles without any complaints.

The other solution I arrived at was aliasing the type and re-exporting it. The only issue with that was more the editor didn't expose the underlying type, but that's not a big deal. Can revert to that if it works.

@moshen
Copy link
Owner

moshen commented Apr 17, 2024

@bvobart
Copy link
Author

bvobart commented Apr 17, 2024

Hmmmmm, this is strange. I tried out your working example and it worked just fine. I was trying to compare between my work project and your example, also to make a MWE showing the bug, but now somehow the issue is fixed. Perhaps there was still an old version around on my system somehow, I don't know.

Anyways, it magically started working and my CI is happy too.

Thanks a lot for your help and your quick replies! 😊

@bvobart bvobart closed this as completed Apr 17, 2024
@moshen
Copy link
Owner

moshen commented Apr 17, 2024

Hmmmmm, this is strange. I tried out your working example and it worked just fine. I was trying to compare between my work project and your example, also to make a MWE showing the bug, but now somehow the issue is fixed. Perhaps there was still an old version around on my system somehow, I don't know.

Oh I believe you. I was getting my own weirdness, highlighted by the multiple pushes above. In some instances, my LSP would report an error, and tsc wouldn't, then I'd make a change, and they would switch. Not sure what exactly is going on with that.

Anyways, it magically started working and my CI is happy too.

Excellent!

Thanks a lot for your help and your quick replies! 😊

My pleasure. Please feel free to ping me if you have any more issues or questions.

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 a pull request may close this issue.

2 participants