-
Notifications
You must be signed in to change notification settings - Fork 28
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
fix: add ".js" extension to StringUtils import #15
Conversation
Thanks for the PR @KevinNovak, could it be a problem for users that don't use ESM? |
I ran a test against both a CommonJS (non-ESM) project and ESM project. This change works on both types of projects. |
@KevinNovak good work, this solved an issue I'm having! @tonivj5 any chance we can get this merged in? |
Looking forward for this PR to be merged 😄 |
Nice job figuring this out 👍 Any chance to get this merged anytime soon? The latest TypeORM Release (0.2.42) added ESM support which apparently leads to a breaking change when using |
FYI while waiting for this PR to be merged, the interim fix is to freeze
{
...
"dependencies": {
...
"typeorm": "0.2.41",
...
}
} |
Can confirm that the suggested fix from @earthpyy works. |
I just moved code from this repo to my project and add this change - works fine |
I'm going to merge this PR and publish as 3.0 👍🏻 |
Released as https://github.com/tonivj5/typeorm-naming-strategies/releases/tag/3.0.0 🚀 npm ref: https://www.npmjs.com/package/typeorm-naming-strategies/v/3.0.0 Thanks to @KevinNovak! The compat with ESM is being a bit hard these days... 😅 |
@tonivj5 I tried updating to the newly released package and I get a compilation error:
We target commonjs as shown above, without a bundler (we just run tsc on the *.ts files). |
typeorm itself actually released a patch that resolved the problem, you probably don't need the change from this PR |
Yep I have the latest patch included (I use a fork from latest typeorm master). Maybe the patch overlaps with this PR? |
yeah try go back to v2 of this package, you'll likely see your problem fixed already |
Same problem here with commonjs (NestJS project) |
Notice the double @coyoteecd & @diogodomanski I wonder if node or other part of your setup is appending |
If the typeorm patch fixes the root cause, I agree this PR should be reverted. |
@KevinNovak could you test if using v2 and latest typeorm version works with your ESM project? |
@tonivj5 Yes, the patch from typeorm v0.2.43 fixes the issue when using typeorm-naming-strategies v2.0.0.
{
"dependencies": {
"typeorm": "0.2.43",
"typeorm-naming-strategies": "2.0.0"
}
} |
I'm going to release a new one without this PR... Thanks for check it @KevinNovak 👍🏻 |
https://github.com/tonivj5/typeorm-naming-strategies/releases/tag/4.0.0 is out, reverting this change, so it's the same than |
This change adds a ".js" file extension to the "StringUtils" import, which makes this project compatible with ESM projects.
Without this change, if you try building with an ESM project you will get the error: