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

jest-worker: should not expose .default babel interop #5803

Closed
ljharb opened this issue Mar 15, 2018 · 22 comments · Fixed by #10623
Closed

jest-worker: should not expose .default babel interop #5803

ljharb opened this issue Mar 15, 2018 · 22 comments · Fixed by #10623

Comments

@ljharb
Copy link
Contributor

ljharb commented Mar 15, 2018

import a from 'jest-worker' of course, works fine, because it's using babel on both ends.

However, const b = require('jest-worker') results in a !== b, and to get at the proper value, you need to do b.default.

Entry points should not expose babel interop details like .default - can https://github.com/facebook/jest/blob/master/packages/jest-worker/src/index.js use module.exports = instead of export default (or can you use the add-module-exports transform?

@SimenB
Copy link
Member

SimenB commented Mar 16, 2018

add-module-exports sounds good to me, I agree it's annoying having to do .default in node (and probably not good for real ESM support in node).

PR welcome! 😀

@felippenardi
Copy link

@SimenB Here is a proposal: #5811

@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

See comment in PR

@SimenB SimenB closed this as completed Mar 19, 2019
@ljharb
Copy link
Contributor Author

ljharb commented Mar 19, 2019

After a year with an open PR, i think this is a really user-hostile decision.

@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

The PR didn't work properly. Using the plugin was discussed a bit in #7554 and it seems like people have been burnt by add-module-exports before. And e.g. Babel itself removed it for their 7 release.

@ljharb
Copy link
Contributor Author

ljharb commented Mar 19, 2019

There’s lots of alternatives including a manual entry point that does the un-.defaulting.

As for Babel themselves, it’s reasonable for Babel to expect consumers to couple to Babel interop, it’s not reasonable for “not Babel” to do so.

@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

Yeah, that's an option. Do you know if that would still work for people doing import? I guess we could do module.exports = require('./build/index'); (unpacking the default in some way) and as long as types in package.json still pointed to build/index.d.ts it would work?

@ljharb
Copy link
Contributor Author

ljharb commented Mar 19, 2019

Yes, it would work for both.

@ljharb
Copy link
Contributor Author

ljharb commented Mar 19, 2019

enzyme does this with all it top-level entry points, for example.

@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

That sounds perfect then. Reopening 😀

@SimenB SimenB reopened this Mar 19, 2019
@JakeMaldonado
Copy link

JakeMaldonado commented Nov 27, 2019

The link at the top is broken. Should be updated to: https://github.com/facebook/jest/blob/master/packages/jest-worker/src/index.ts

@anje123
Copy link
Contributor

anje123 commented Oct 7, 2020

@SimenB @jeysal I would love to take this up as my first issue if possible

@anje123
Copy link
Contributor

anje123 commented Oct 9, 2020

@ljharb can i use this https://www.npmjs.com/package/babel-plugin-add-module-exports or do i need to explicitly, change to export default to module.exports

@ljharb
Copy link
Contributor Author

ljharb commented Oct 9, 2020

@saenglert as long as the main entry point of the package only has one default export, that transform should do the job.

@anje123
Copy link
Contributor

anje123 commented Oct 9, 2020

@ljharb please i dont really understand your explanation do you mean this transformation export default to module.exports and not using the babel package

@SimenB
Copy link
Member

SimenB commented Oct 9, 2020

We're exporting multiple things (which I don't wanna change), so the babel plugin won't work. A named export is probably our only option, although it's a breaking change. Instead of export default class JestWorker we can do export class JestWorker. Possibly name it Worker instead as we don't need to repeat the package name in the export

@ljharb
Copy link
Contributor Author

ljharb commented Oct 9, 2020

@SimenB i'm pretty sure you could still change things by attaching all the named exports to the default export, and explicitly add __esModule: true, and it wouldn't be a breaking change - it'd be pretty gross tho, to be sure.

@SimenB
Copy link
Member

SimenB commented Oct 10, 2020

Just ditch the default export, I don't think it's worth it to go through weird hoops to support both and keep TypeScript happy.

@anje123
Copy link
Contributor

anje123 commented Oct 11, 2020

@SimenB I changed it to the named export, got all tests passing at the jest-worker package and build successful but i cant access jest-worker outside the package using a named import
Link to my branch: https://github.com/MLH-Fellowship/jest/tree/fix-export-default-worker
test-worker

@SimenB
Copy link
Member

SimenB commented Oct 11, 2020

@anje123 if you open up a PR with your WIP stuff I can take a look 🙂

@anje123
Copy link
Contributor

anje123 commented Oct 11, 2020

@SimenB created a PR #10623 Thanks

@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 11, 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.

5 participants