refactor: use full relative paths in imports

[wolfy1339]

This should help bring ESM compatibility in the future

[wolfy1339]

This works fine with the TS compiler itself, but not Jest

[gr2m]

This works fine with the TS compiler itself, but not Jest

If jest is the problem, I'm okay with moving away from it. I heard from others that Jest is a big pain point in the transition to ESM

[wolfy1339]

If jest is the problem, I'm okay with moving away from it. I heard from others that Jest is a big pain point in the transition to ESM

ESM support is not in the current stable release of jest; To get ESM support, you need jest@next and ts-jest@next
The tests work fine without any modifications when run as ESM.

---

Here's what I have found with some testing:

• Removing the .js from imports within the tests resolves the files correctly, however the imports in the source code fail to resolve. From what it looks like, jest is trying to resolve these imports when it should be ts-node resolving the imports
• The "express middleware no mount path no next" test no longer works, because we cannot use require() in ESM which is used to import express without the types, because importing express with types results in the test never finishing (it times out). Commenting out the webhooks node middleware makes it work.

[gr2m]

importing express with types results in the test never finishing (it times out)

this is the weirdest thing! Is this related to jest or do you see the same when you just import express in an ESM node script?

[G-Rath]

I'd personally prefer to stick with jest - We're planning to ship the new major of jest in a couple of months, so shouldn't be too long a wait; using next in the meantime would help weed out any bugs :)

[gr2m]

I'd personally prefer to stick with jest - We're planning to ship the new major of jest in a couple of months, so shouldn't be too long a wait; using next in the meantime would help weed out any bugs :)

ok 👍🏼

We're planning to ship the new major of jest

You are working on jest?

[G-Rath]

You are working on jest?

I mainly maintain eslint-plugin-jest, but do contributions to jest itself too; but so I know a few of the folks there (and specifically the guy who handles the releases who also helps maintain the eslint plugin).

[wolfy1339]

This is the test in question:

https://github.com/octokit/webhooks.js/blob/master/test/integration/node-middleware.test.ts#L360-L390

It only fails in this test, and only when you import with types for express. Wether in ESM or CommonJS
The middleware should've called the next function, since it isn't the path it is using, but it doesn't seem like it did and the middleware just hangs.

EDIT: Only happens in jest@next and ts-jest@next

[wolfy1339]

The next() function from express is indeed getting called when you give an invalid route.

The test stalls right after we call the next() function.

EDIT: something to do with the versions of jest and ts-jest that is used. The current versions (in master) don't exhibit this behaviour

[wolfy1339]

@G-Rath do you know what could be causing this?
As soon as we use jest@next withts-jest@next and install @types/express the tests stall

[G-Rath]

@wolfy1339 not off the top of my head, but could try looking into it this weekend.

However, the fact that the issue happens when installing @types/express makes me think of an issue caused by the type of Request looks like that code was removed here.

[wolfy1339]

I think we are running into this issue: kulshekhar/ts-jest#2010

[wolfy1339]

I have tried forcing ts-jest to output ESM, but that doesn't help either.

@G-Rath Can you help out here to figure out a way to go?

[wolfy1339]

I did some more testing, found a fix.

• This problem only manifests itself when using TypeScript and you are using the types for express (ie not using require)
• The problem arises when the middleware encounters a path it doesn't know about
• Specifying the middleware first before any other fixes this problem

It seems the reverse of #534 but only when paths are unknown to the middleware

[oscard0m]

I did some more testing, found a fix.

• This problem only manifests itself when using TypeScript and you are using the types for express (ie not using require)
• The problem arises when the middleware encounters a path it doesn't know about
• Specifying the middleware first before any other fixes this problem

It seems the reverse of #534 but only when paths are unknown to the middleware

Just to clarify: Is the problem in our end and how we define types or is something maybe we should report to jest?

[wolfy1339]

Just to clarify: Is the problem in our end and how we define types or is something maybe we should report to jest?

I am really not sure what the underlying issue really is.

I only know that the issue doesn't arise if it is the first in the chain of middleware, and only happens when you try to access a path that isn't handled by the Webhooks middleware.

The types don't seem to be anything wrong with them that would conflict like this

[oscard0m]

Just to clarify: Is the problem in our end and how we define types or is something maybe we should report to jest?

I am really not sure what the underlying issue really is.

I only know that the issue doesn't arise if it is the first in the chain of middleware, and only happens when you try to access a path that isn't handled by the Webhooks middleware.

The types don't seem to be anything wrong with them that would conflict like this

Do you think it would be interesting to try to reproduce it without Octokit (a small codesanbox maybe?)? This way we could know if it's something related to Octokit or to jest under concrete circumstances. Only if it is not a big effort to create this simplified example.

[wolfy1339]

Do you think it would be interesting to try to reproduce it without Octokit (a small codesanbox maybe?)? This way we could know if it's something related to Octokit or to jest under concrete circumstances. Only if it is not a big effort to create this simplified example.

Yes, it would probably be helpful to diagnose the issue to create a simplified example.

We'd just need to extract the middleware logic from this module.

[oscard0m]

I was discussing with some friends the status of jest regarding ESModules. Do you know what's the status on that? Is this the issue I should subscribe: jestjs/jest#11167?

[wolfy1339]

Jest should work fine with plain JS using ESM.

The only issue we have, is because of TypeScript and the way Jest handles imports (kulshekhar/ts-jest#2010), since the file index.js doesn't exist, jest complains that it cannot find the import. It is valid TypeScript and the compiler deals with the file extensions.

Since we are going to be moving to pure ESM + external TypeScript definition files, most of the changes in this PR are moot.

[ollie-iterators]

I assume this will get unblocked by #819.

[wolfy1339]

I don't think it will have an effect

[ollie-iterators]

I thought that #819 was using "esbuild to transpile the TS source code into an ESM source ...". Does that mean that it should only unblock it once the changes go to the other areas in the repository?

[wolfy1339]

Yes, we do build for ESM. However, those builds are meant for the browser/deno and we are still using CommonJS for NodeJS
Until work on octokit/octokit-next.js is complete we are still using CommonJS

[wolfy1339]

I came back to this PR, and I have gotten it working

[G-Rath]

Looks good to me! Just one question: looks like we're using .ts for type imports and .js for runtime code in the source code but not in tests - I'm figuring that's because the tests go through an extra step somewhere that handles that? but my actual question is are we able to use .js for the type imports too, and if so I think we should do that for consistency

(and sorry for not responding to your last ping on this - I don't recall ever seeing it in my inbox 😬)

[wolfy1339]

Looks good to me! Just one question: looks like we're using .ts for type imports and .js for runtime code in the source code but not in tests - I'm figuring that's because the tests go through an extra step somewhere that handles that?

It's a functionality included in recent versions of TypeScript using the allowImportingTsExtensions option.

but my actual question is are we able to use .js for the type imports too, and if so I think we should do that for consistency

We are absolutely able to use the .js extension

[github-actions]

🎉 This PR is included in version 12.0.8 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀