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

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

Merged
merged 1 commit into from Mar 9, 2023

Conversation

huntie
Copy link
Contributor

@huntie huntie commented Mar 7, 2023

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
Contributor

mrazauskas commented Mar 7, 2023

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
Contributor Author

huntie commented Mar 7, 2023

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

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

@SimenB
Copy link
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
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
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.

e2e/resolve-conditions/custom-env.js Outdated Show resolved Hide resolved
packages/jest-environment-jsdom/src/index.ts Outdated Show resolved Hide resolved
packages/jest-environment-node/src/index.ts Outdated Show resolved Hide resolved
@huntie huntie force-pushed the custom-env-override-conditions branch from a42ae3d to 67be705 Compare March 9, 2023 17:08
@huntie
Copy link
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.

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.

👍

@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
Member

SimenB commented Mar 12, 2023

@huntie is this something you need released?

@huntie
Copy link
Contributor Author

huntie commented Mar 13, 2023

@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
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

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
Member

SimenB commented Jul 4, 2023

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.

None yet

4 participants