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

feat(jest-environment-{node,jsdom}): allow specifying customExportConditions #12774

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Apr 29, 2022

Allow customExportConditions in both of our environments.
Tested using e2e.
Documented in an awkward but the only place.

@SimenB
Copy link
Member

SimenB commented Apr 29, 2022

Mention here? https://jestjs.io/docs/configuration#testenvironmentoptions-object

Only suggestion

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

### Features

- `[jest-environment-jsdom]` Allow specifying `customExportConditions` instead of the default `'browser'` ([#12774](https://github.com/facebook/jest/pull/12774))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add support to node env as well?

@jeysal
Copy link
Contributor Author

jeysal commented Apr 29, 2022

Yeah that's the only place where it could make sense to mention, especially if we add support for node too. Slightly awkward though since they're environment-specific and thus all just "example" mentions

@jeysal jeysal force-pushed the jsdom-custom-export-conditions branch from 6ba206f to 0783267 Compare April 29, 2022 13:50
@jeysal jeysal changed the title feat(jest-environment-jsdom): allow specifying customExportConditions instead of the default 'browser' feat(jest-environment-{node,jsdom}): allow specifying customExportConditions Apr 29, 2022
@jeysal jeysal force-pushed the jsdom-custom-export-conditions branch from 0783267 to 8480107 Compare April 29, 2022 13:56
@jeysal
Copy link
Contributor Author

jeysal commented Apr 29, 2022

@SimenB added a lil mention to docs, and made node env also support it

@jeysal jeysal requested a review from SimenB April 29, 2022 13:59
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff

@SimenB
Copy link
Member

SimenB commented Apr 29, 2022

Oh, maybe tweak "override" wording https://jestjs.io/docs/upgrading-to-jest28#packagejson-exports

@jeysal jeysal force-pushed the jsdom-custom-export-conditions branch from 8480107 to d7a6ba6 Compare April 29, 2022 14:08
@jeysal jeysal merged commit c8c32d3 into main Apr 29, 2022
@SimenB SimenB deleted the jsdom-custom-export-conditions branch April 29, 2022 14:30
F3n67u pushed a commit to F3n67u/jest that referenced this pull request May 2, 2022
@SimenB
Copy link
Member

SimenB commented May 6, 2022

@josundt
Copy link

josundt commented May 11, 2022

@SimenB It would be nice if the initial value of JSDomEnvironment.customExportConditions in jest-jsdom-environment was changed from ['browser'] to ['jsdom', 'browser'].

Rationale:
node, browser and jsdom are three different runtime environments with different properties (where jsdom really is a full node environment with an partly emulated browser environment on top).

In the jsdom runtime context, most of the time you want a package's "browser" conditional export, but other times it is better to use the "node" conditional export, since jsdom is missing certain browser features like crypto or fetch.

It would be nice if cross-env package authors were able to add a conditional "jsdom" export to package.json to direct to the correct implementation when running the code in a jsdom runtime context, f.ex. when running Jest.

We actually already added "jsdom" conditional exports in addition to "node" and "browser" for our internal packages to make them work with Jest unit tests, and for now we set jest.config's testEnvironmentOptions.customExportConditions to ['jsdom', 'browser'].

But I believe this could have been the initial value for JSDomEnvironment.customExportConditions.
It would be a non-breaking change that would simplify making cross-env packages work in node, browsers and tests.

@SimenB
Copy link
Member

SimenB commented May 11, 2022

I don't think we'll add any defaults beyond ones specified here: https://nodejs.org/api/packages.html#community-conditions-definitions

@josundt
Copy link

josundt commented May 11, 2022

@SimenB I can live with that, I can anyway configure it manually in jest.config.js.

But personally I still think it would be correct to express this environment condition, since the runtime environment in my opinion is not correctly identified by "browser" or "node".

FYI: Webpack is utilizing non-standard custom conditions quite extensively to enable advanced features:
https://webpack.js.org/guides/package-exports/#conditions

@github-actions
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 Jun 11, 2022
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.

4 participants