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

Add warning in development when a result function is x => x. #645

Merged
merged 24 commits into from
Dec 1, 2023

Conversation

aryaemami59
Copy link
Contributor

@aryaemami59 aryaemami59 commented Nov 26, 2023

This PR:

  • Adds identityFunctionCheck to warn to the user if the result function returns its input. It has the same configuration options as inputStabilityCheck.
  • Adds unit tests for identityFunctionCheck.
  • Adds JSDocs for identityFunctionCheck.
  • Adds documentation for identityFunctionCheck to README.
  • Adds DevModeChecks, a type representing the configuration for development mode checks.
  • Replaces inputStabilityCheck and identityFunctionCheck inside CreateSelectorOptions with devModeChecks.
  • Adds DevModeChecksExecutionInfo, a type representing execution information for development mode checks.
  • Adds setGlobalDevModeChecks, a function allowing to override globalDevModeChecks.
  • Creates devModeChecks folder.
  • Moves each dev-mode-check to their own file.
  • Removes shouldRunDevModeCheck in favor of getDevModeChecksExecutionInfo.
  • Updates unit tests to comply with these changes.
  • Adds documentation to Netlify docs once merged with Transition README Docs to Docusaurus-Netlify Setup #641.
  • Updates README with new dev-mode-check API.

Copy link

codesandbox-ci bot commented Nov 26, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ae0a921:

Sandbox Source
Vanilla Typescript Configuration

Copy link
Contributor

@EskiMojo14 EskiMojo14 left a comment

Choose a reason for hiding this comment

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

haven't had a chance to look through this properly yet, but a few initial suggestions

src/utils.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/createSelectorCreator.ts Outdated Show resolved Hide resolved
@aryaemami59
Copy link
Contributor Author

BTW I only called it noopCheck to stay consistent with react-redux, if you have a better suggestion I'm all ears :)

@EskiMojo14
Copy link
Contributor

we couldn't come up with anything better for react-redux so i doubt we'll do so here :) noopCheck is fine imo

@aryaemami59
Copy link
Contributor Author

aryaemami59 commented Nov 26, 2023

we couldn't come up with anything better for react-redux so i doubt we'll do so here :) noopCheck is fine imo

How about identityFunctionWarning or maybe noTransformationWarning?

@aryaemami59 aryaemami59 marked this pull request as ready for review November 26, 2023 14:06
src/utils.ts Outdated Show resolved Hide resolved
@markerikson
Copy link
Contributor

I don't know how many more dev checks we'd add in the future, but I think we ought to consolidate this into a single combined setter function just in case we do add more, rather than adding a bunch of individual setters.

I'm thinking something like setDefaultDevCheckFrequency(checks: Partial<Record<DevCheckType, DevCheckFrequency>>).

Let me play with that a bit on this branch.

@aryaemami59
Copy link
Contributor Author

I don't know how many more dev checks we'd add in the future, but I think we ought to consolidate this into a single combined setter function just in case we do add more, rather than adding a bunch of individual setters.

I'm thinking something like setDefaultDevCheckFrequency(checks: Partial<Record<DevCheckType, DevCheckFrequency>>).

Let me play with that a bit on this branch.

Yeah, I agree. If I put them in different files, would that be problematic?

@markerikson
Copy link
Contributor

I'm picturing having a singleton object with the current global check values, and the setter would loop over the incoming options and assign. (Could even do Object.assign(globalOptions, incomingOptions) if we aren't worried about someone passing in {stability: undefined}, I guess.)

@EskiMojo14
Copy link
Contributor

the "should run" check explicitly checks against "always" and "once" so any other values will just function the same as "never", which i think is okay

@aryaemami59
Copy link
Contributor Author

aryaemami59 commented Nov 26, 2023

@markerikson I think I get it, so it would look something like this:

export type DevModeChecks = Record<
  'inputStabilityCheck' | 'identityFunctionCheck',
  DevModeCheckFrequency
>

const globalDevModeChecks: DevModeChecks = {
  inputStabilityCheck: 'once',
  identityFunctionCheck: 'once'
}

export const setGlobalDevModeChecks = (
  devModeChecks: Partial<DevModeChecks>
) => {
  Object.assign(globalDevModeChecks, devModeChecks)
}

then inside createSelectorCreator:

      memoize,
      memoizeOptions = [],
      argsMemoize = defaultMemoize,
      argsMemoizeOptions = [],
      inputStabilityCheck = globalDevModeChecks.inputStabilityCheck,
      identityFunctionCheck = globalDevModeChecks.identityFunctionCheck
    } = combinedOptions

and to set the values globally:

it('disables check if global setting is changed', () => {
    setGlobalDevModeChecks({ inputStabilityCheck: 'never' })

    expect(addNums(1, 2)).toBe(3)

    expect(unstableInput).toHaveBeenCalledTimes(1)

    expect(consoleSpy).not.toHaveBeenCalled()

    setGlobalDevModeChecks({ inputStabilityCheck: 'once' })
  })

This could actually work I think. It passed the tests.

@aryaemami59 aryaemami59 marked this pull request as draft November 27, 2023 15:44
test/defaultMemoize.spec.ts Show resolved Hide resolved
test/defaultMemoize.spec.ts Show resolved Hide resolved
README.md Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
test/weakmapMemoize.spec.ts Show resolved Hide resolved
@aryaemami59
Copy link
Contributor Author

aryaemami59 commented Nov 29, 2023

@markerikson I'm not too sure about the warning message in the console, do you think we can improve on it, or leave it as is?

@aryaemami59 aryaemami59 marked this pull request as ready for review November 30, 2023 06:19
aryaemami59 and others added 14 commits November 30, 2023 22:31
- Add `DevModeChecks`, a type representing the configuration for development mode checks.
- Replace `inputStabilityCheck` and `identityFunctionCheck` inside `CreateSelectorOptions` with `devModeChecks`.
- Add `DevModeChecksExecutionInfo`, a type representing execution information for development mode checks.
- Add `setGlobalDevModeChecks`, a function allowing to override `globalDevModeChecks`.
- Create devModeChecks folder.
- Move each dev-mode-check to their own file.
- Remove `shouldRunDevModeCheck` in favor of `getDevModeChecksExecutionInfo`.
- Update unit tests to comply with these changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants