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

Setup proper ESM Support #79

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Setup proper ESM Support #79

merged 3 commits into from
Feb 21, 2024

Conversation

alexcaza
Copy link
Owner

@alexcaza alexcaza commented Feb 19, 2024

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
Copy link

egmacke commented Feb 21, 2024

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
Copy link

egmacke commented Feb 21, 2024

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
Copy link
Owner Author

alexcaza commented Feb 21, 2024

@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
Copy link

egmacke commented Feb 21, 2024

@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
Copy link
Owner Author

@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!

@alexcaza alexcaza merged commit f6e22a2 into main Feb 21, 2024
2 checks passed
@alexcaza alexcaza deleted the fix-78 branch February 21, 2024 17:49
@egmacke
Copy link

egmacke commented Feb 22, 2024

@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?

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.

Error when using in ESM project
2 participants