Skip to content

fix: Fix user override of customExportConditions in custom env subclasses#13989

Merged
SimenB merged 1 commit intojestjs:mainfrom
huntie:custom-env-override-conditions
Mar 9, 2023
Merged

fix: Fix user override of customExportConditions in custom env subclasses#13989
SimenB merged 1 commit intojestjs:mainfrom
huntie:custom-env-override-conditions

Conversation

@huntie
Copy link
Copy Markdown
Contributor

@huntie huntie commented Mar 7, 2023 •
edited
Loading

Summary

Adjusts behaviour in jest-environment-jsdom and jest-environment-node (no breaking changes).

With customExportConditions exposed as a public property on each env class, and as previously tested in deno.test.mjs, subclasses can override "exports" conditions in two ways:

image

Previously, both overriding the property or reimplementing exportConditions() would mean that when a user attempted to override this in Jest config, this was ignored.

{
  testEnvironment: 'react-native/jest/react-native-env', // (via preset: 'react-native')
  testEnvironmentOptions: {
    customExportConditions: ['test', 'react-native'],
  },
}

These changes make sure this behaviour works for the property assignment case — and is covered by tests. We plan to make this load-bearing in the React Native Jest preset for 0.72.

Changes:

  • Updates jest-environment-jsdom, jest-environment-node to use the customExportConditions property as a default, which will be overridden if set in config.projectConfig.testEnvironmentOptions.
  • Renames deno.test.mjs/deno-env.js to custom-env (more generic).
  • Adds custom-env-override-conditions.test.mjs with new test case (tests via @jest-environment-options doc comment).

Test plan

yarn jest e2e/__tests__/resolveConditions.test.ts

✅ Updated tests pass

@huntie huntie changed the title test: Add test for user-overridden customExportConditions via a custom env test: Add test for user-overridden customExportConditions Mar 7, 2023
@mrazauskas
Copy link
Copy Markdown
Contributor

mrazauskas commented Mar 7, 2023 •
edited
Loading

Would be useful to add configuration example you provided to the testEnvironmentOptions option documentation. Or is that clear? Hm.. Perhaps it is clear? (At first it sounded like this is new feature.)

@huntie
Copy link
Copy Markdown
Contributor Author

huntie commented Mar 7, 2023 •
edited
Loading

@mrazauskas Believe this is present, but happy to improve.

image

Note: ⚠️ Incoming tweak to this PR, as I forgot the .test.js suffix on the new test and it wasn't being run!

@mrazauskas mrazauskas mentioned this pull request Mar 8, 2023
Merged
@mrazauskas
Copy link
Copy Markdown
Contributor

mrazauskas commented Mar 8, 2023

I took one more look and opened a PR to adding the examples. See #13991

@SimenB
Copy link
Copy Markdown
Member

SimenB commented Mar 8, 2023

Note: ⚠️ Incoming tweak to this PR, as I forgot the .test.js suffix on the new test and it wasn't being run!

@huntie is it? 😀

@huntie
Copy link
Copy Markdown
Contributor Author

huntie commented Mar 9, 2023

So it turns out that neither of these patterns allows overriding via projectConfig.testEnvironmentOptions...

image

...because the property initialiser customExportConditions is assigned after the constructor runs (overwriting the parent class constructor) 😐.

  1. ⬆️ For this reason, customExportConditions should perhaps be made private.
  2. To get the same functionality, the extending class must merge the config object with any defaults and call the parent constructor (this provides an API free of the above footgun, without breaking changes, but is less intuitive and awkward in practice)
    • or, we update both NodeEnvironment and JSDOMEnvironment implementations to resolve the customExportConditions value in the accessor method instead. ⬅️ This approach is incoming.

@huntie huntie force-pushed the custom-env-override-conditions branch from 499117d to 738e982 Compare March 9, 2023 12:59
@huntie huntie changed the title test: Add test for user-overridden customExportConditions feat: Update env packages to allow overriding customExportConditions in subclasses Mar 9, 2023
@huntie huntie force-pushed the custom-env-override-conditions branch from 738e982 to a42ae3d Compare March 9, 2023 13:10
@huntie huntie changed the title feat: Update env packages to allow overriding customExportConditions in subclasses fix: Fix user override of customExportConditions in custom env subclasses Mar 9, 2023
@huntie
Copy link
Copy Markdown
Contributor Author

huntie commented Mar 9, 2023

PR is updated and now makes a small functional fix along with test updates (see previous comment), preferring a non-breaking approach.

@mrazauskas @SimenB Ready for review.

SimenB
SimenB approved these changes Mar 9, 2023
View reviewed changes
Comment thread e2e/resolve-conditions/custom-env.js
Comment thread packages/jest-environment-jsdom/src/index.ts Outdated
Comment thread packages/jest-environment-node/src/index.ts Outdated
@huntie huntie force-pushed the custom-env-override-conditions branch from a42ae3d to 67be705 Compare March 9, 2023 17:08
@huntie
Copy link
Copy Markdown
Contributor Author

huntie commented Mar 9, 2023

67be705:

  • Add custom-env-conditions-method-override.js and related test, covering original test case of overridden exportConditions() method.

Ready for review.

SimenB
SimenB approved these changes Mar 9, 2023
View reviewed changes
Copy link
Copy Markdown
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.

👍

@SimenB SimenB merged commit 26bd03a into jestjs:main Mar 9, 2023
@huntie huntie deleted the custom-env-override-conditions branch March 9, 2023 19:36
@SimenB
Copy link
Copy Markdown
Member

SimenB commented Mar 12, 2023

@huntie is this something you need released?

@huntie
Copy link
Copy Markdown
Contributor Author

huntie commented Mar 13, 2023 •
edited
Loading

@SimenB Would be nice, but it's not super urgent — especially as Package Exports is shipping experimentally at first. Not a direct dependency for the config updates in React Native, just the edge case will get fixed under the next version :).

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 16, 2023
@huntie @facebook-github-bot
Update custom "exports" conditions in Jest environment
0a3c555 
Summary:
Updates the default set of `"exports"` condition names in our `ReactNativeEnv` for Jest, so that it aligns with the defaults in React Native CLI (react-native-community/cli#1862).

Also includes a subtle update to how this is accomplished. Instead of overriding `exportConditions()`, we assign to the underlying class property — this allows users (once jestjs/jest#13989 is merged) to override `customExportConditions` via [`testEnvironmentOptions`](https://jestjs.io/docs/configuration#testenvironmentoptions-object).

```js
  preset: 'react-native',
  testEnvironmentOptions: {
    customExportConditions: ['test', 'react-native'],
  },
```

Changelog: [Internal]

Reviewed By: jacdebug

Differential Revision: D43879056

fbshipit-source-id: 86fffe2b5fdf9d8492d25b8b12a78be75b5fa3be
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 13, 2023

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 Apr 13, 2023
@SimenB
Copy link
Copy Markdown
Member

SimenB commented Jul 4, 2023

https://github.com/jestjs/jest/releases/tag/v29.6.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@SimenB SimenB SimenB approved these changes

Assignees

No one assigned

Labels

cla signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@huntie @mrazauskas @SimenB @facebook-github-bot