watcher
watcher copied to clipboard
fabiospampinato
Fix TypeScript issues when using `moduleResolution` of `node16`
Seeing a similar issue to #33 when trying to import Watcher into Immich which also uses node16 resolution.
Error I see is "The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'."
This PR may resolve this issue.
@milesmahon I just took a look at the Immich server. The other Immich sub-projects are ESM, so it's unfortunate that one's still on CJS. However, you can use ESM code inside CJS code, but you need to use a dynamic import rather than static import. I.e. what you can do is await import('watcher') inside the watch method in storage.repository.ts. That means you will have to change watch to be async and update the tests. I'm not sure if that will cause much complication, but hopefully it doesn't
This PR won't help on the CJS/ESM front, but might help you squash some type checking errors.
@fabiospampinato any thoughts on whether this could be merged? My experience is that if you're on a recent version of TypeScript, then you're best off using node16 for libraries because it's the strictest and will help it be consumed in the most projects (and conversely if you're working on an end application you should consider using bundler as it's the most accepting and will allow you to use the widest variety of libraries)
@benmccann I'm just not sure about the implications here, I don't see the benefits of adding extensions to imports in source files, when the compiled library already has import extensions.
And I don't know which problem switching to node16 actually fixes, if that's actually better presumably I should switch to that in tsex/tsconfig.json directly.
Ah, gotcha. I'd missed your comment on the issue. Let me take a look at providing more info around whether it can be reproduced or should be closed. I assumed there was a problem because of the open issue and just wanted to help out, but I'll take a look at confirming that
@fabiospampinato I was able to reproduce it and provided more details on the issue. Any chance you'd be able to take another look at this PR now?
@benmccann yes, you identified an actual problem I had missed. This is a bug in tsex which doesn't seem to be transforming those imports in declaration files correctly, I need to get it fixed there and then we can check if there's still a problem. Btw I started a new job recently, so I'm not exactly at full capacity for open-source at the moment, but I'll get this fixed 👍
Is there anything, I can do to help? Does anyone know why it's just failing on Linux if just the imports were changed?
The test failures on Linux are unrelated to this PR. If you have a Linux machine and would be able to debug the failures, a PR to address them would be much appreciated!
Started to take a look at the issues. Not sure how helpful I will be but I will try my best. First of all, while running the tests on Windows ran into an issue with this test. touch is not a command on Windows. Not sure how this test is passing on the GitHub Actions. Should we disable this test if on Windows?
Hmm, all the tests pass on the Windows CI and that test has been there for 3 years (https://github.com/fabiospampinato/watcher/commit/e318a30e124b45170f4600b2254dcfd5e2cb62ad), so I feel like it probably doesn't need to be disabled. Perhaps @fabiospampinato knows how it works and if there's some guidance that could be provided for Windows contributors running the tests locally on their machines
This shouldn't be necessary anymore in the next release. Thanks though.