jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.12k stars 6.44k forks source link

5 builtins missing from `jest-resolve` #4677

Closed billiegoose closed 6 years ago

billiegoose commented 6 years ago

Expected result: tests pass Actual result:

  ● Test suite failed to run

    Cannot find module '_stream_transform' from 'index.js'

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:191:17)
      at Object.<anonymous> (node_modules/thru/index.js:1:106)

These are builtin modules that are part of the undocumented "private" core API but are unfortunately shimmed in browserify, and some module authors continue to prefer them over the public builtins because using them shaves a few kb off of their bundle size:

_stream_duplex
_stream_passthrough
_stream_readable
_stream_transform
_stream_writable

My root problem is that the test-runner, jest, does not recognize _stream_transform as a core module and breaks as a result. You can see I originally tried to submit a fix to thru (the module that requires _stream_transform) but as switching from _stream_transform to stream would have increased the bundle size, I am now looking for other solutions. builtin-modules is philosophically opposed to adding them to it's list. is-builtin-module is by the same author. So I've worked my way up the dependency chain to jest-resolve. Hopefully you will see that practicality of adding these to the list of builtin modules, since browserify makes this assumption already.

cpojer commented 6 years ago

Feel free to send a PR to check for these in Jest.

billiegoose commented 6 years ago

I'll give it a shot!

SimenB commented 6 years ago

We should whitelist all with an underscore here: https://github.com/nodejs/node/tree/master/lib

Fix should be here: https://github.com/facebook/jest/blob/0748e6f76394b58ce24aee42e08c96fb509e8744/packages/jest-resolve/src/index.js#L199

billiegoose commented 6 years ago

We should whitelist all with an underscore

Awesome! @SimenB that was the solution I was trying, but I got stuck on something. Happy to see somebody figured it out. 😄

SimenB commented 6 years ago

BTW, requiring _stream_transform et.al. will 100% break in future versions of node (deprecation warning likely in node 9), so relying on it is not a good idea. I'm happy that the solution in #4740 will automatically support whatever is in the running version of node, so nothing needs to happen from Jest's side. This is more a warning for consumers

https://github.com/nodejs/node/pull/11957

billiegoose commented 6 years ago

I'm happy that the solution in #4750 [sic] will automatically support whatever is in the running version of node

I'm going to double check that with - dang Twitter is fast, James already replied https://twitter.com/jasnell/status/923895172705824768

github-actions[bot] commented 3 years ago

This issue 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.