search

alexcaza / export-to-csv

Export a JS collection to CSV; written in TypeScript.
Other
200 stars 48 forks source link

Setup proper ESM Support #79

Closed alexcaza closed 7 months ago

alexcaza commented 7 months ago

Previously there was an issue with the settings in tsconfig.json, what configs were being used by tsc and how bun bundled the project.

Closes #78

egmacke commented 7 months ago

I've just been trying to test this against my own project (having forked it) but I'm getting type errors when trying to run the build: image

egmacke commented 7 months ago

My only other comment (and this ties into the failing tests) is that the generateCsv return type is CsvOutput which is a wrapped type. However both your tests, and I suspect most users, will be expecting this to actually return a string.

In the library there is an unpack function which handles this, but it's not exported from the package. Can I suggest either exporting that (with some docs) or changing the return type of generateCsv to string by calling unpack?

alexcaza commented 7 months ago

@egmacke Yeah, there already is! You can use asString which is exported from the library. It's in the documentation for Nodejs, but it's definitely not clear! I'll update the readme with the example of how to use the output from generateCsv as a string. Thanks for bringing it up 😄

Edit: I'm not sure why this type error is happening in the tests for you. These issues never presented themselves locally...

egmacke commented 7 months ago

@alexcaza thanks - sorry I missed the bit in the docs!

Regarding the tests - the issue seems to be because you're comparing CsvOutput objects to strings - maybe I've got something set more globally in my environment to enforce stricter typechecking, but it was a clean checkout, then npm install, npm run build that resulted in the errors

alexcaza commented 7 months ago

@egmacke no worries! Still worth making it clearer.

Yeah, I'll dig into it, because that was the expectation I had during development and was surprised it was lifting the new type to a string. What's confusing is that the GH actions I have setup to run unit tests pass as well, so it's likely a config option somewhere that's off.

I'll address this in another PR!

egmacke commented 7 months ago

@alexcaza thanks for getting this resolve nice and quick!

With regards the CI not failing - I've had a quick look, and I can't see any point where you build the code in CI. The errors are compile time so wouldn't show up just running the tests.

Can I suggest you tweak you CI to include a build step before running the tests?