feat: add default exportConditions for both jsdom and node environments

[piranna]

Summary

#11863 adds support for a new optional exportConditions() method in custom environments, so it's possible to add extra conditions. In its tests, it add as an example the browser and node conditions to custom environments that extends from jest-environment-jsdom and jest-environment-node respectively. In normal execution, node extra condition is provided by default by Node.js itself, and although not provided by Node.js, browser is one of the Node.js endorsed ones.

This pull-request modify the jest-environment-jsdom and jest-environment-node environments to provide the browser and node conditions by default.

Test plan

This code is a copy&past of the actual one in the tests, so instead of being used in ad-hoc child classes on the tests or for demo purposses, they are provided as default values for everybody to use.

As a side note, with the PR now that ad-hoc classes could be removed and use the unmodified jest-environment-jsdom and jest-environment-node that this PR provides, so I can update the PR to remove them too, if you want.

[codecov-commenter]

Codecov Report

Merging #11924 (ef89ae8) into main (a9b7e8a) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #11924      +/-   ##
==========================================
- Coverage   67.25%   67.24%   -0.02%     
==========================================
  Files         329      329              
  Lines       17353    17357       +4     
  Branches     5074     5074              
==========================================
  Hits        11671    11671              
- Misses       5650     5654       +4     
  Partials       32       32              

Impacted Files	Coverage Δ
packages/jest-environment-jsdom/src/index.ts	72.13% <0.00%> (-2.45%)	⬇️
packages/jest-environment-node/src/index.ts	70.49% <0.00%> (-2.39%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9b7e8a...ef89ae8. Read the comment docs.

[piranna]

I've removed the custom environment classes from tests to use the new ones that already provides the conditions, so this way they are actually being tested and code coverage doesn't decrease, and also it's removed duplicated code.

[SimenB]

Hey! This isn't safe to do as node means "anything node can load" which means .node or .json files (which isn't supported by ESM) and ESM files (which isn't supported by require). You can make this decision in your own project, but Jest itself cannot as it depends on user config.

We can probably add a way to configure this so the conditions can be provided by config, but just setting them by default won't work.

browser is safer, but would create a weird asymmetry imo

[SimenB]

Actually, due to #11919 (comment) we might need to do this. Still needs to go in a major release tho

[piranna]

This isn't safe to do as node means "anything node can load" which means .node or .json files (which isn't supported by ESM) and ESM files (which isn't supported by require).

But, that's exactly the purposse of node export condition, to load "anything node can load", no matter it's format... Also, according to Node.js docs, node seems to be used only to load CommonJS or ESM modules, and for .node files you need to use node-addon condition, that I've not added it in this PR but probably we should for completeness. In any case, they "should" be defined in the exports field after import and require entries, so since they are resolved in order by the spec, jest would never get there, and if so, you can have nested conditions to later explicit set if you want a require or an import.

According to Node.js docs, this is the expected behaviour and usage, so I think it's totally save to add this PR functionality.

[piranna]

Still needs to go in a major release tho

No issues on that, fine for me :-)

[piranna]

Is this ready to be merged for the upcoming Jest 28?

[piranna]

Any update on this? What needs to be done so it gets merged? It has lasted two months now...

[piranna]

Should node-addon condition be added too?

[piranna]

Any update on this? It seems like k-g-a/jest-node-exports-resolver#9 is directly caused by the lacking of node condition, that this PR fixes... How can I help to get it merged?

[piranna]

Any update on this? How can we ver It merged?

[SimenB]

when we start landing breaking changes

[piranna]

Tests are passing except for a Windows odd one, seems to be a random failure unrelated to this PR. Otherwise, this is ready to be merged.

[SimenB]

thanks! I'll land this when I start landing breaking changes (hopefully next week)

[SimenB]

sweet, thanks!

[SimenB]

I'm not allowed to make changes to this PR btw, so when the time comes to merge it, I'll need you to resolve conflicts. If you can give me access (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) I can handle any conflicts myself 🙂

[piranna]

I'm not allowed to make changes to this PR btw, so when the time comes to merge it, I'll need you to resolve conflicts. If you can give me access (docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) I can handle any conflicts myself

It doesn't shows me that option, maybe are they only for maintainers? It shows you are only as contributor... Anyway, no problem on do the conflicts resolving myself, just ping me :-)

[SimenB]

I'm allowed in essentially every single PR, don't think I've experienced not being able to outside of deleted branches/forks.

Probably because you've opened the PR from the main branch instead of branching off

[piranna]

Probably because you've opened the PR from the main branch instead of branching off

It would made sense. Anyway, just ping me and I'll do any needed changes, no issues :-)

[SimenB]

Cool 👍 I'll start landing breaking changes at some point this week

[piranna]

Cool I'll start landing breaking changes at some point this week

Great :-)

[SimenB]

@piranna if you rebase now, I think it should be good to land tomorrow 🙂 No current plans to land any features before this, so in theory no conflicts

[piranna]

@piranna if you rebase now, I think it should be good to land tomorrow No current plans to land any features before this, so in theory no conflicts

Rebase, or just fix conflicts? I've just done the second one.

[SimenB]

Yeah, whatever is easiest for you 🙂 Merging is fine! We squash on merge anyways, so doesn't really matter how messy the commit history on the branch is 😀

[piranna]

Then you already have it :-)

[SimenB]

Thanks!

[piranna]

Welcome! :-D

[SimenB]

https://github.com/facebook/jest/releases/tag/v28.0.0-alpha.0

[github-actions]

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.