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

5 builtins missing from jest-resolve #4677

Closed
billiegoose opened this issue Oct 12, 2017 · 7 comments · Fixed by #4740
Closed

5 builtins missing from jest-resolve #4677

billiegoose opened this issue Oct 12, 2017 · 7 comments · Fixed by #4740

Comments

@billiegoose
Copy link

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
Copy link
Member

cpojer commented Oct 12, 2017

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

@billiegoose
Copy link
Author

I'll give it a shot!

@SimenB
Copy link
Member

SimenB commented Oct 21, 2017

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
Copy link
Author

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
Copy link
Member

SimenB commented Oct 27, 2017

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

nodejs/node#11957

@billiegoose
Copy link
Author

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
Copy link

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.

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

Successfully merging a pull request may close this issue.

3 participants