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

Support for using native Node ESM loader #13521

Closed
wants to merge 3 commits into from

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Oct 26, 2022

Summary

As previously discussed in #13143, this PR shows roughly what changes would be needed in order to support Node.js --loader. I'm opening this as a draft PR since I would need guidance on where the code should live, how the code should be activated by the user, and how to create tests for it.

ping @SimenB

I would be very happy to receive any feedback, or to discuss how this could/should work!

Test plan

I've tested this by running yarn build and manually coping the changed files into another small test project 🙈

package.json

{
  "type": "module",
  "dependencies": {
    "@esbuild-kit/esm-loader": "^2.5.0",
    "jest": "^29.2.2"
  },
  "jest": {
    "extensionsToTreatAsEsm": [
      ".ts"
    ]
  }
}

foobar.test.ts

test('it works', (): void => {
  expect(1).toBe(1)
})
cp ~/coding/jest/packages/jest-worker/build/workers/ChildProcessWorker.js node_modules/jest-worker/build/workers/ChildProcessWorker.js
cp ~/coding/jest/packages/jest-transform/build/NodeEsmLoaderTransformer.js node_modules/@jest/transform/build/NodeEsmLoaderTransformer.js
cp ~/coding/jest/packages/jest-transform/build/index.js node_modules/@jest/transform/build/index.js
cp ~/coding/jest/packages/jest-runner/build/runTest.js node_modules/jest-runner/build/runTest.js

NODE_OPTIONS='--experimental-vm-modules --loader @esbuild-kit/esm-loader' jest

Screenshot 2022-10-27 at 01 00 00

I have currently only tested this with @esbuild-kit/esm-loader, but the goal is that it should work with any --loader

@LinusU LinusU marked this pull request as draft October 26, 2022 23:06
@LinusU
Copy link
Contributor Author

LinusU commented Oct 26, 2022

Pushed a small fix that makes it work with ts-node/esm as well 🎉

Screenshot 2022-10-27 at 01 13 48

@LinusU
Copy link
Contributor Author

LinusU commented Oct 27, 2022

Pushed another small fix that uses the built in loader to load the config file as well 🎉

edit: didn't actually pushed it, but pushed now 🙈

@LinusU
Copy link
Contributor Author

LinusU commented Oct 31, 2022

(rebased on #13543)

@LinusU
Copy link
Contributor Author

LinusU commented Nov 8, 2022

(rebased on #13543)

@LinusU
Copy link
Contributor Author

LinusU commented Jan 4, 2023

@SimenB @privatenumber @mariocandela

Is there anything that can be done from my side to move forward with this? ☺️

@LinusU
Copy link
Contributor Author

LinusU commented Feb 1, 2023

@SimenB @privatenumber @mariocandela

Would love to move forward with this! Anything I can do from my side?

@privatenumber
Copy link

Nothing more from me, but I appreciate that you keep pushing.

I think you'll want to ping @SimenB directly as he's the maintainer.

@shawnmcknight
Copy link

@LinusU Thanks for taking a crack at this. Being able to use any loader will be helpful and a step in the right direction.

One thing I'm wondering is whether you ran through any scenarios using tsconfig path mapping? With tsx supporting that out of the box, I think any solution here would likely need to also support that functionality. I might be out of my element here, but from evaluating tsx's code base, it seems that the path mapping works via the resolve method on the loader. With your proposed solution that effectively uses the loader as a transformer, I'm concerned that the loader might not be getting the actual correct filepath without calling the loader's resolve as well.

I might be way off base here, and everything you've proposed might work fine there, but I thought it would be worth broaching the subject in case it wasn't considered.

The other thing that comes to mind here is the possibility that multiple loaders have been specified. If I'm following your solution correctly, only the first loader will be processed. That would likely cover a huge percentage of any use case for loaders, but there could be scenarios where multiple loaders are necessary and it would not work.

Thanks again for your efforts to land this improvement!

@LinusU
Copy link
Contributor Author

LinusU commented Feb 7, 2023

@LinusU Thanks for taking a crack at this. Being able to use any loader will be helpful and a step in the right direction.

Happy to help! 🙏

One thing I'm wondering is whether you ran through any scenarios using tsconfig path mapping? With tsx supporting that out of the box, I think any solution here would likely need to also support that functionality. I might be out of my element here, but from evaluating tsx's code base, it seems that the path mapping works via the resolve method on the loader. With your proposed solution that effectively uses the loader as a transformer, I'm concerned that the loader might not be getting the actual correct filepath without calling the loader's resolve as well.

I haven't personally used tsconfig path mapping, but you're probably right in that they wouldn't work. However, I'm not sure that they work when using tsx as loader anyways. tsx is a standalone binary that adds a lot of features on top of node. My guess is that path mapping only works with that standalone binary. Would you be able to test this? e.g. by chancing tsx to node --loader tsx and running your app.

So if my hunch is correct, I'm not sure that it's inside the scope to support this, since this is about supporting any --loader and not specifically tsx.

The other thing that comes to mind here is the possibility that multiple loaders have been specified. If I'm following your solution correctly, only the first loader will be processed. That would likely cover a huge percentage of any use case for loaders, but there could be scenarios where multiple loaders are necessary and it would not work.

This is not supported currently, but should probably be quite easy to add in. Would prefer to do it in a follow up PR though to limit the scope of this one.

Thanks again for your efforts to land this improvement!

❤️

@LinusU
Copy link
Contributor Author

LinusU commented Feb 7, 2023

(rebased on latest master)

@LinusU
Copy link
Contributor Author

LinusU commented Feb 7, 2023

@SimenB I think that the only thing I need in order to take this out of the draft stage is some input on how this should be activated. The code currently checks if there is a --loader argument present in NODE_OPTIONS, which is very convenient for people that are already using loaders in their projects, and just want it to work™.

However, I'm thinking that this could be a breaking change if someone is currently passing in a loader and relying on it not being picked up.

Another option could be to add a --loader flag to the Jest cli. In fact, even if we also look in NODE_OPTIONS it could be nice to also be able to pass a loader via the cli, same as in Node.

Do you have any input on this?

@privatenumber
Copy link

privatenumber commented Feb 7, 2023

@LinusU
Copy link
Contributor Author

LinusU commented Feb 7, 2023

@privatenumber thanks for the link! It seems like one more improvement here would be to support the resolve hook as well!

Personally, I would prefer it if that would be done in a follow up to keep this PR as tight in scope as possible. But if this is deemed as required in order to support loaders than we can add it here.

To keep track, there is currently two "extra" features that would be nice:

@LinusU
Copy link
Contributor Author

LinusU commented Feb 7, 2023

It seems like the linting rules was recently updated, I had to make the following changes to make it happy, but I'm not sure that it is desirable. Maybe I misunderstood something?

     __: string,
     ___: ReducedTransformOptions,
   ): Promise<TransformResult> {
-    throw new Error(
-      '`transformSourceAsync` should not be called when using --loader',
+    return Promise.reject(
+      new Error(
+        '`transformSourceAsync` should not be called when using --loader',
+      ),
     );
   }
       url,
       {format: 'module', importAssertions: {}},
       async () => {
-        return {format: 'module', source: fileSource};
+        return Promise.resolve({format: 'module', source: fileSource});
       },
     );

These two are async functions, but they don't use await in them. The rule triggered was @typescript-eslint/require-await.

       );
     }
 
-    return await requireOrImportModule(moduleName);
+    return requireOrImportModule(moduleName);
   }
 }

Removing await here will remove the current function (NodeEsmLoaderTransformer#requireAndTranspileModule) from stack traces when the code in requireOrImportModule throws an Error. This seems like it removes important information from the stack trace.

More info about this: https://github.com/goldbergyoni/nodebestpractices#-212-always-await-promises-before-returning-to-avoid-a-partial-stacktrace

@koshic
Copy link

koshic commented Mar 16, 2023

@LinusU it's a nice concept, but I'm afraid it looks like a complicated hack to do simple thing: use custom transformer. In other words, you can create transformer.mjs in ~40 lines of code to use any ESM loader just by importing it. With some helper library it might look like this:

// transformer.mjs referenced in jest config
import {wrapper} from 'jest-esm-loader-support';
import {resolve, load} from 'any modern ESM loader';
export default wrapper(resolve, load);

Moreover, because you can pass options to the transformer via https://jestjs.io/docs/configuration#transform-objectstring-pathtotransformer--pathtotransformer-object, it doesn't need to write any code:

{
  transform: {
    "\\.ts$": [require.resolve('jest-esm-loader-support-v2'), {loader: 'any modern ESM loader'}]
  },
}

Sure, you can implement a hacky logic to load currently applied loader (which of them in case of more than 1 loader used?) too if 'loader' parameter is omitted :)

@mrazauskas
Copy link
Contributor

Hm.. Not sure if transformer would be enough. Config files are not passed through transformers, because it is not yet clear which transformers will be needed.

@koshic
Copy link

koshic commented Mar 19, 2023

Hm.. Not sure if transformer would be enough. Config files are not passed through transformers, because it is not yet clear which transformers will be needed.

We have 3 type of entities here:

  1. Jest config file. Works fine with .js / .cjs / .mjs extensions, and use ts-node for .ts. Which is obviously works only in CJS mode. Solution is easy - detect current mode first (package.json "type" field will help) and just import file without node-js tricks for ESM mode. User runtime should handle any extensions (even 'jest -c jest.config.aurebesh' will work) specified by user.
  2. Various files used to extend Jest (custom transformers, setup / teardown, etc.). Currently handled with a bit incorrect requireOrImportModule (as I mentioned in [Bug]: incorrect behaviour of requireOrImportModule from jest-util #14013), but we can assume this is more or lees ok.
  3. Source files. Can be handled with custom transformer on 40 lines which simple uses any ESM loader. One-liner if some helper library used (may be integrated into Jest directly via exported function or indirectly - as special config flag / key / syntax).

Anyway, we just can't grab loader module specifier from argv, import & use it - because loader chaining already landed in Node. It will work only for simple scenarios with 1 loader. For more complex - not, especially if we don't want to re-implement lib/internal/modules/esm/hooks.js logic.

Copy link

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Mar 18, 2024
Copy link

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Apr 17, 2024
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2024
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.

7 participants