-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Bin exported without extension, breaks node's native loader picker and ts-node/esm #4645
Comments
This is an working example with Mocha, ESM and ts-node/esm. |
@juergba The error actually comes from node...
Most symlinks in
nandoid goes as far as using cjs |
I haven't understood yet, or something is wrong in your argumentation.
|
If you remove |
Yes, above sample does not work anymore, when I remove
@cspotcode can you have a look at this, please? |
This is a confirmed bug in nodejs. Please remind them that they need to fix it! I have cited mocha as one of the libraries being broken by it, but it will help if mocha's users and maintainers also remind them (politely) that they need to fix this! To reproduce the bug, use a completely empty
EDIT Fixed a typo above; the |
@cspotcode thank you for your explanation, I will remind nodejs tomorrow. @rayfoss you have the workaround with |
I think if mocha wants to implement a simple fix, they can add a new bin entrypoint file with a file extension (make sure to keep the old one, too, for backwards-compatibility) and then point I believe that update can be published as a non-breaking change. |
... My PR is not a breaking change in any way. There is an argument to be made that it should be |
You mention a drawback in your PR; is that non-breaking? We might be talking about the same thing. I'm imagining that mocha ships 4x different files: |
My PR only addresses the issue at hand, I couldn't see how you guys output bundled code... So the PR has
Symbolic Links made by NPM:
We can make a Update: I corrected the description of the PR to reflect it's contents. I updated the PR code a few hours after publishing to duplicate naked vanilla |
no longer an issue in Mocha 9 |
Hi again @juergba, could it be that the |
@lachrist There is no loader option in Mocha. Just use Edit: Mocha recognizes Node's option and forwards them to Node by spawning a child-process. |
Now, it makes more sense! I got confused because node has no documented Thanks for the swift answer! |
Welcome. |
@juergba @giltayar This issue occurs today. I'm not sure why it was fixed in mocha v9 release but occurs now exactly as OP described for exactly the reason OP described. I spoke with npm and yarn authors, and both package managers automatically alias a non-naked bin with a naked version (but not the other way around). There is an on-going discussion within Node.js on how to handle naked bin files, but it has become rather protracted and does not appear to be resolving any time soon. The quickest solution to get mocha working with loaders is to:
I'd be happy to do this if you agree. |
@JakobJingleheimer no, I'm against this When you set |
Ah. Perhaps a documentation update then? |
@juergba why are you against this? (or at least two |
Also keeping in mind that npm and yarn both handle supplying the naked one automatically. |
@giltayar In aprox. April/May we will publish Mocha v10. If Node still hasn't made up their mind until then about this extensionless binary story, we evtl. could add |
Hi guys I've discussed this issue with the node team. And this will probably never be solved on their end. Checkout nodejs/node#41465. In particular, this is my final take on it:
|
What is the (historic) reason that binaries (or executables?) are extensionless?
For which reason does npm/yarn do that? |
Yes, I'm 99% sure the historic reason is days of yore (unix/linux). Note that adding the Also, I'm completely confident that making the two tiny changes I suggested will 100% be compatible now and in future, wherever loaders lands ;) @lachrist cough I am on the node team 🙂 There is a more recent discussion, but I'm not linking to it at the moment because I need to port it from a PR (that I need to closed) to an actual discussion. |
Technically the execute bit tells Linux it's an executable, and the two-byte sequence Package managers use https://www.npmjs.com/package/@zkochan/cmd-shim to generate |
^^ This is a fully backwards-compatible fix and takes little effort, right? I feel like there's some confusion with talk of this being a breaking change, but it's not. Is there anything we can explain further? |
If you mean: I can live with adding an additional @JakobJingleheimer please go ahead with your PR, if you are still on fire. Otherwise we wait till Mocha@10. |
Yes, exactly 🙂
You don't have to do this: npm and yarn will take care of it automatically.
Not on fire. I've just seen quite a few bugs about it. Whatever you prefer 🙂 I've consolidated the discussions into nodejs/node#41711 |
This is not true. Take a look at e.g. nanoid after installation with npm, there is a |
Yeah some tools have hardcoded paths to the extensionless JS file, so just
as easy to keep it there and add an extension-ed file that requires it.
(Or vice versa, either works)
…On Thu, Jan 27, 2022, 1:27 PM Juerg B. ***@***.***> wrote:
You don't have to do this: npm and yarn will take care of it automatically.
This is not true. Take a look at e.g. nanoid after installation with npm,
there is a bin/nanoid.cjs but no bin/nanoid.
—
Reply to this email directly, view it on GitHub
<#4645 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC35OCZCFLDEB42M26PVULUYGFANANCNFSM46AJASBQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
ts-node 10.6.0 implements a workaround that hopefully mimics the bugfix which will eventually be published in node core. This means that, for users of mocha and ts-node, upgrading to the latest ts-node should avoid this issue. https://github.com/TypeStrong/ts-node/releases/tag/v10.6.0 |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [ts-node](https://typestrong.org/ts-node) ([source](https://github.com/TypeStrong/ts-node)) | devDependencies | minor | [`10.5.0` -> `10.6.0`](https://renovatebot.com/diffs/npm/ts-node/10.5.0/10.6.0) | --- ### Release Notes <details> <summary>TypeStrong/ts-node</summary> ### [`v10.6.0`](https://github.com/TypeStrong/ts-node/releases/v10.6.0) [Compare Source](TypeStrong/ts-node@v10.5.0...v10.6.0) Questions about this release? Ask in the official discussion thread: [#​1666](TypeStrong/ts-node#1666) **Added** - Adds workaround for extensionless entrypoints with ESM loader ([#​1649](TypeStrong/ts-node#1649), [#​1654](TypeStrong/ts-node#1654)) - You can now combine tools such as `mocha` with `--loader ts-node/esm`, where previously node would throw `[ERR_UNKNOWN_FILE_EXTENSION]` - node has a bug where combining `--loader` with an extensionless entrypoint causes this error [nodejs/node#​33226](nodejs/node#33226) - Some tools, for example `mocha`, have an extensionless entrypoint. ([source](https://github.com/mochajs/mocha/blob/547ffd73535088322579d3d2026432112eae3d4b/package.json#L37), [source](https://github.com/mochajs/mocha/blob/547ffd73535088322579d3d2026432112eae3d4b/bin/mocha)) - Combining `NODE_OPTIONS=--loader ts-node/esm` with these tools causes this error. [mochajs/mocha#​4645](mochajs/mocha#4645) - node intends to fix this bug in a future release: [nodejs/node#​41711](nodejs/node#41711) - In the interim, we have implemented a workaround in ts-node. - Adds support for target "ES2022" in `moduleTypes` overrides ([#​1650](TypeStrong/ts-node#1650)) **Fixed** - Fixed bug where `--swc` and other third-party transpilers did not respect `moduleTypes` overrides ([#​1651](TypeStrong/ts-node#1651), [#​1652](TypeStrong/ts-node#1652), [#​1660](TypeStrong/ts-node#1660)) - Fixed bug where node flags were not preserved correctly in `process.execArgv` ([#​1657](TypeStrong/ts-node#1657), [#​1658](TypeStrong/ts-node#1658)) - This affected `child_process.fork()`, since it uses `process.execArgv` to create a similar child runtime. - With this fix, `child_process.fork()` will preserve both node flags and `ts-node` hooks. - Fixed compatibility TypeScript 4.7's API changes ([#​1647](TypeStrong/ts-node#1647), [#​1648](TypeStrong/ts-node#1648)) https://github.com/TypeStrong/ts-node/milestone/9 </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1195 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Description
custom loaders like ts-node have a really hard time figuring out the mocha "binary"
The binary lacks.js or cjs extensions. Changing the
node_modules/.bin/mocha
extension isn't enough.Steps to Reproduce
npm run broken-test-npx
or runsource env.sh; mocha test/test.js
OR
npm install ts-node typescript @types/mocha mocha
on atype: "modules"
project.jsonexport NODE_OPTIONS="--loader ts-node/esm/transpile-only"
npx mocha test/any-test-at-all.js
Expected behavior:
No errors, tests run.
Actual behavior:
Setting a loader prevents node from running the mocha binary, in any way
Reproduces how often: 100% of the time
Versions
node --version
: 14 (Sandbox) and 16 as wellAdditional Information
Transpile only mode does not make a difference. Extension of tests do not make a difference. As long as the loader boots up, when node proceeds to guess which loader to use, node errors out.
The text was updated successfully, but these errors were encountered: