Skip to content
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

feat: expose node.js loader #9

Merged

Conversation

DylanPiercey
Copy link
Contributor

@DylanPiercey DylanPiercey commented May 17, 2022

Resolves #8

Exposes the underlying --require and --loader hooks from @esbuild-kit/cjs-loader and @esbuild-kit/esm-loader respectively.

Note: I think this is all good but I have not actually tested it yet.

@DylanPiercey
Copy link
Contributor Author

Looks like the cjs --require hook isn't working since the package is type: "module". Need to figure out how to make pkgroll output .cjs files.

@DylanPiercey
Copy link
Contributor Author

I've updated the exports to expose ./loader for the --loader and ./register for the --require hook. This seems to work well (previously it was causing an issue in mocha specifically which tried to use esm first).

@privatenumber
Copy link
Owner

Thanks for the PR @DylanPiercey

I would actually prefer the original syntax you had so it can be used like:

node -r tsx --loader tsx

Can you try something like this?

{
	...,
	"exports": {
		".": {
			"require": "./dist/loader/cjs.cjs",
			"import": "./dist/loader/esm.mjs"
		}
	},
	...
}

You can test the change after pushing it to GitHub with: build-this-branch.

@privatenumber
Copy link
Owner

Just pulled in the latest develop which makes a change to the export map.

@DylanPiercey
Copy link
Contributor Author

@privatenumber I did test with trying to have a single . export. However the problem is I'm personally trying to use this as a require hook with mocha which currently has some strange rules around supporting esm, like not supporting it in watch mode. The silly thing is that when you pass a --require flag to mocha it will always try to import() it first, and fallback to require(). Because of this I couldn't actually install the require hook and so I thought exposing them independently might be better.

Maybe there is a another option here though. I wonder if we could only expose an esm version and have it do something like this:

import { createRequire } from "module";
createRequire(import.meta.url)("@esbuild-kit/cjs-loader");

// export all of the loader hooks also:
export const resolve /** ... */

I don't have time to test this just yet.

@DylanPiercey
Copy link
Contributor Author

@privatenumber turns out I did have time.

I tested it and it seems to work great!
Essentially now when you import tsx from esm it will register both the loader and the require hook. If you import from cjs then it only adds the require hook.

@DylanPiercey
Copy link
Contributor Author

One thing to consider but I'm not sure if it should be included in this, is that we may want to update the following to use the internal imports rather than directly using @esbuild-kit/*:
https://github.com/esbuild-kit/tsx/blob/develop/src/run.ts#L38

Copy link
Owner

@privatenumber privatenumber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Can you run the linter? npm run lint -- --fix

@@ -0,0 +1,3 @@
import { createRequire } from "module";
export * from "@esbuild-kit/esm-loader";
createRequire(import.meta.url)("@esbuild-kit/cjs-loader");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI with pkgroll, you should be able to just do require('@esbuild-kit/cjs-loader')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that this is loaded from nodes esm environment, but still adds the cjs require hook so that if you manually createRequire it will still work. I don't think the above suggestion would work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it should be fine to import a cjs module which should also work. Never mind!

Copy link
Owner

@privatenumber privatenumber May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@DylanPiercey DylanPiercey May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we even need that though? In theory this is just an import to a cjs module which should just work right?

In theory I can just have

export * from "@esbuild-kit/esm-loader";
import "@esbuild-kit/cjs-loader";

@DylanPiercey
Copy link
Contributor Author

Will be a few hours before I can update the commit. I'll double check that everything still works w/o using createRequire or require. If that doesn't work I'll just use require as you've said.

@DylanPiercey
Copy link
Contributor Author

@privatenumber I simplified as I described above and it still works. Also ran the lint/format. LMK if anything else is missing 😄

README.md Outdated Show resolved Hide resolved
@DylanPiercey
Copy link
Contributor Author

@privatenumber I was just able to test this again, and actually I realized that with the current pattern of having the esm version both export the loader api, and register the require hook, it will still work as mocha -r tsx since the -r flag will work fine with the esm api.

I've gone ahead and simplified the PR now to only have a single loader.js and a simple . export.

@privatenumber
Copy link
Owner

Awesome 👍 Glad we reached an elegant solution!

I removed the reference to mocha's --require, -r flag because they implement a custom one that supports ESM. This can be confused with Node.js's --require, -r flag, which only supports CommonJS.

@DylanPiercey
Copy link
Contributor Author

This can be confused with Node.js's --require, -r flag, which only supports CommonJS.

Ah I assumed node did the same and allowed esm here. So that means if there is a case where you want to use this with cjs only then it wouldn't work. So maybe what was there was better?

@privatenumber
Copy link
Owner

For CJS only use-cases, cjs-loader should be used directly.

tsx should only be used when requiring both ESM and CJS support.

@DylanPiercey
Copy link
Contributor Author

@privatenumber that's fair enough. Just personally like this as a one tool to work in both use cases.
As it stands, given mocha's special handling of -r this works for me though.

@privatenumber
Copy link
Owner

privatenumber commented May 18, 2022

Currently, tsx does work in both use-cases (Node.js and mocha).

(I think it would be quite rare and edge-casey for someone to have to use cjs-loader instead of tsx)

Do you know any other Node.js binaries that accept -r where this API wouldn't work?

@@ -0,0 +1,6 @@
// Hook require() to transform to CJS
require('@esbuild-kit/cjs-loader');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to require() from import.

When using import, the ESM loader ends up calling require after determining it's a CJS package. Calling require() directly short-circuits it.

Copy link
Owner

@privatenumber privatenumber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to merge if this looks good to you @DylanPiercey

@DylanPiercey
Copy link
Contributor Author

@privatenumber not sure off the top of my head but I know I've seen some cli tools that forward node options through besides mocha in the past and presumably this would apply to all of them unless they special cased like mocha.

If you're concerned about the complexity there maybe let's just leave it since that should be an additive change anyway

@privatenumber
Copy link
Owner

Don't really follow what you're trying to say... but will move forward with this tomorrow morning.

BTW, I took a closer look at what mocha's doing. I recommend passing tsx in via env variable so that it can intercept both require and import. With -r, the loader API is ignored.

@DylanPiercey
Copy link
Contributor Author

DylanPiercey commented May 19, 2022

@privatenumber mocha has limited support for esm and so I was using it in cjs mode (eg watch mode does not work). For this case I actually do exclusively want the require hook.

However I think it works fine in cases where you just need the require hook to use the loader with what we've done here. Actually in mocha it works to use mocha --loader tsx even for cjs stuff, so I think that's the way to go.

@privatenumber privatenumber changed the title feat: expose loader and require hook feat: expose node.js loader May 19, 2022
@privatenumber privatenumber merged commit a271f4d into privatenumber:develop May 19, 2022
@privatenumber
Copy link
Owner

🎉 This PR is included in version 3.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@DylanPiercey DylanPiercey deleted the loader-and-require-hook branch May 19, 2022 16:03
@DylanPiercey
Copy link
Contributor Author

🎉 thanks for working with me on this. Look forward to switching some projects to tsx!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose --require and --loader hook from this module.
2 participants