Update loader.ts #24
Update loader.ts #24
Conversation
|
Thanks for the pull request! Would you be able to add a test for this? Should be able to simply spy |
|
Also needs |
lib/loader.ts
Outdated
| const tsNodeInstance = register({ ...options, compilerOptions: { module: "commonjs" } }) | ||
| return (path: string, content: string) => { | ||
| try { | ||
| // cosmiconfig requires the transpiled configuration to be CJS | ||
| register({ ...options, compilerOptions: { module: "commonjs" } }).compile( | ||
| tsNodeInstance.compile( |
There was a problem hiding this comment.
Yes, been on PTO, will get them asap, thanks!
There was a problem hiding this comment.
awesome, thank you, enjoy your PTO :)
There was a problem hiding this comment.
I'm confused by the failing test "rethrows an error if it is not an instance of Error", what are we testing here?
There was a problem hiding this comment.
So, unfortunately, you can throw anything in JS. You can throw 0, throw "silly string", or throw {}. This is important to note because within cosmiconfig-typescript-loader we do not have control over what errors may be thrown by ts-node.
If something fails and throws an error that is not an Error (of the Error prototype) we do not want to try and construct a TypeScriptCompileError from it.
We check if the thrown error is an instance of Error, and if so construct a TypeScriptCompileError from this with fromError.
If it is not, then we simply rethrow what was thrown to prevent a more confusing error from being raised and masking the original error.
There was a problem hiding this comment.
I've looked into this and have got the test working again, a small mocking update :)
The test passes if we update the beforeEach of the "ts-node" test suite:
beforeEach(() => {
stub = jest.spyOn(tsnode, "register").mockImplementation(
() =>
({
compile: (): string => {
// eslint-disable-next-line @typescript-eslint/no-throw-literal
throw unknownError;
},
} as any)
);
loader = TypeScriptLoader();
});This involves changing the mock to now return an object that is service-like since we are now needing compile to throw the error instead of register.
This raises another concern I have which is since the tsNodeInstance setup has been moved out of the try/catch the behaviour has changed in a rather significant way. We should still handle errors being thrown during the call to register in a similar way, might take some more thinking.
In regards to releasing these changes, once we resolve these things I will probably major version bump this due to the nature of the changes.
There was a problem hiding this comment.
What could throw errors from the register, I feel like a misconfiguration of the loader should be handled differently from the errors throw from compile, as loader is going to fail for the developer setting up the loader rather than later based on a random config file issue.
There was a problem hiding this comment.
Makes sense for a major, while not an API change, it could impact current users negatively.
lib/loader.spec.ts
Outdated
| @@ -9,12 +9,13 @@ describe("TypeScriptLoader", () => { | |||
| const fixturesPath = path.resolve(__dirname, "__fixtures__"); | |||
|
|
|||
| let loader: Loader; | |||
| let tsNodeSpy = jest.spyOn(tsnode, "register"); | |||
There was a problem hiding this comment.
This is constant, and only used in one test so can be moved to the test itself:
it("should use the same instance of ts-node across multiple calls", () => {
const tsNodeSpy = jest.spyOn(tsnode, "register");
|
I had some thoughts about how we can bring this in and keep everyone happy. It's a long weekend here so I'll look into this again in the next few days :) I really appreciate what you've proposed so far 🚀 |
Codecov Report
@@ Coverage Diff @@
## main #24 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 26 27 +1
Branches 2 2
=========================================
+ Hits 26 27 +1
Continue to review full report at Codecov.
|
|
Going to be looking at this properly again today sometime :) |
|
Published under 2.0 :) apologies for the delays. |
Only create a single instance of ts-node.
We load many configs, we noticed a significant memory leak, this creates a single instance of ts-node