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

Broken on TypeScript #763

Closed
RubberDuckShobe opened this issue May 30, 2023 · 12 comments · Fixed by #784 or #800
Closed

Broken on TypeScript #763

RubberDuckShobe opened this issue May 30, 2023 · 12 comments · Fixed by #784 or #800

Comments

@RubberDuckShobe
Copy link

Hello, I am trying to use this package with TypeScript, but starting my application fails. I posted the log on Pastebin, because it's too long to include here: https://pastebin.com/Dn7VT1gG

To reproduce this, simply trying to run any Node project using this TypeScript with this package should be enough.

@RubberDuckShobe
Copy link
Author

Seems it's only broken with strict enabled.

@spiff-radio
Copy link

spiff-radio commented Jun 19, 2023

it would be nice not to have to disable strict :)

  • your package makes mine completly crash when running npm run build. Computer freezes, etc.
    If I remove the package, it builds in less than one second.
    There something strange there :)

Capture d’écran du 2023-06-19 23-32-10

@Haschikeks
Copy link
Collaborator

Hey @spiff-radio,

your error seems really strange, as it says JavaScript heap out of memory.
I'm not sure if this is the same problem. Does your build run if you disable strict?

@spiff-radio
Copy link

This is actually what happens if I disable strict ;)

@nfadili
Copy link

nfadili commented Oct 29, 2023

I am running into this issue as well using this library in a project that has strict mode enabled. I believe the overall issue here is that .ts files are being included in the published npm module and are exported alongside the .js files. This means that when projects import from this package using typescript, the typescript modules are actually imported and thus are compiled and checked alongside the source code of the project.

The way around this in my experience is building your .js and .d.ts files into a dist folder before publishing and then having your package.json reference that dist folder like this:

{
  // ... other fields
  "main": "dist/musicbrainz-api.js",
  "types": "dist/musicbrainz-api.d.ts",
}

Currently, your compiled project files as well as your source code is all contained in the lib directory. By separating your compiled files you will allow projects to import only what is expected for distribution. You can also control the final size of you npm module a bit if you want.

If you are open to this change I'd be happy to take a stab at. It might necessitate a major version bump though because existing typescript projects might already be relying on importing ts modules directly...? Not quite sure.

@Borewit
Copy link
Owner

Borewit commented Nov 5, 2023

If this project would be elevated to strict type script, I expect you issues would disappear.

@nfadili
Copy link

nfadili commented Nov 7, 2023

@Borewit configuring typescript to build in strict mode would solve the immediate problem yeah! I still believe that it'll be easier to maintain long-term by building and shipping your compiled distribution code in the NPM package and not included your typescript source files.

  • It retains all the benefits of typescript (intellisense, compile-time typechecking, etc.)
  • No chance of conflicts between typescript projects (the current issue)
  • The NPM module will be more portable overall just being locked to ES5 js which can be recognized in any kind of project. For example: if you wanted this module to be usable on the browser it can not contain .ts files.
  • Your package size can be reduced

My PR #783 gets a functional version of this type of build process, but it likely interrupts your dev workflow and such so I am not sure I can take it much farther. I would be happy to help with this if it is of interest to you. For the time being I am using a fork of this package so that I can use it in my typescript projects.

@Borewit
Copy link
Owner

Borewit commented Nov 7, 2023

I still believe that it'll be easier to maintain long-term by building and shipping your compiled distribution code in the NPM package and not included your typescript source files.

I totally agree, in fact I was not aware of that I did, that is simply a bug.

@Borewit
Copy link
Owner

Borewit commented Nov 7, 2023

Let me know what is remaining in this distribution v0.11.0 @nfadili .

@nfadili
Copy link

nfadili commented Nov 7, 2023

@Borewit I just installed v11 and it works as expected! Your solution to exclude ts via the files property in package.json did the trick.

Thank you for the quick and helpful work on this! I will close my PR 👍

@Borewit
Copy link
Owner

Borewit commented Nov 7, 2023

Thanks for pointing out this problem @nfadili . I applied this way convention of distribution configuration in most of my projects, but apparently I forgot to adapt this part in this repository.

@gordielachance
Copy link

working now, thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants