fix: change tsconfig and package exports for types to match output #71
Conversation
Also removed unneeded dependencies that we can just use node inbuilts as we only support node >= 16. Had to switch jest -> vitest as jest couldn’t handle the node ESM import format correctly in tests. Fixed tslint errors as well.
| import { Factory } from '../src/factory'; | ||
| import { TestSuite } from '../src/test_suite'; | ||
| import { TestCase } from '../src/test_case'; | ||
| import { Factory } from '../src/factory.js'; |
There was a problem hiding this comment.
Why is this importing js file and not typescript files?
There was a problem hiding this comment.
To answer both of your questions in one place; this is required for "module": "Node16" and no bundler (ie no rollup) when targeting ESM compatible output. So basically the output of tsc is exactly what needs to be run in nodejs. Typescript throws an error with the module/target config combination needed for proper JS output for running in node without an extra bundler. There's more detail in this Stack Overflow but the short of it is that ESM nodejs scripts have to include the extension.
Now why this is .js and not .ts is that node can't run typescript files, so the output that node sees is essentially the output of tsc, meaning at that point all the "files" are .js not .ts. Jest (without a bundler in the middle) and it's ts-node just try and import the .js file in the src not the "compiled" output, so it can't find the file. But changing this to .ts and the node.js output (and tsc) will fail (actually maybe this is more ts-node? I couldn't be bothered figuring out why it didn't work just used the same solution as last time but it's definitely "typescripts" issue not jest).
As you mentioned, including some kind of bundler makes this a whole lot easier - jest essentially has an internal bundler - as they mangle all the imports to make them correct before spitting them out. Here we're just using tsc and not bothering with a bundler so it's a bit more inconvenient. Case of less abstractions/steps means more weirdness just staring you in the face... The bundler approach is definitely more forgiving and easier!
Vitest (and vite) is just newer than Jest so they're using newer tools under the hood that are more "aware" of ESM without having to handle all the older commonjs issues that came out when Jest was growing up I guess.
Confusing right, hope some of that helps explain a bit 😂
There was a problem hiding this comment.
Thanks for the clarification!
Is there any reason to not use a bundler (except that this PR already exists)?
There was a problem hiding this comment.
I was just going for minimal changes to make the existing changes work, moving to a bundler would have been working at the problem from essentially starting from scratch rather than modifying what's there in my mind.
I know of no reason not to use a bundler, except maybe less dependencies/simpler setup.
Seeing that other issue just raised about v4, maybe that's a good reason to need a bundler so we can export both a CJS and ESM build, in this PR I was just fixing the ESM exports as was done in the typescript upgrade. (or rather what I thought had been done, maybe it wasn't intentional!)
There was a problem hiding this comment.
All right, that's reasonable. And thanks again for another contribution!
The change was not intentional.
|
Can you elaborate on what didn't work with jest and why the imports are using .js? Just out of curiosity, as I maintain a few packages with jest and typescript (and rollup) and I don't have these issues... |
|
P.S. I'm not a maintainer of this lib. |
|
I don't mind working on a PR to introduce a bundler (I like rollup, but I don't mind adding a different one if there's a different preference). |
|
@HarelM If you're happy to do that it sounds like it's probably for the best. With this PR, and it seems unintentionally with the v4 upgrade, we broke compatibility with CJS usage so adding a bundler that can do the esm + cjs exports would be best. I don't mind closing off this PR in favour of yours 😁 |
|
Let me open a PR first, then you can review it and decide which is better. |
|
@HarelM, yes, I'll hold a little while. I have no strong preference when it comes to bundlers, and Rollup seems like a very good choice. Thank you both again for all of your help! |
|
I've opened a PR, see #73 |
|
Closing this for #73 |
Closes #24