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

Corrected documented type for defaultValue #2778

Merged
merged 7 commits into from
Apr 23, 2020

Conversation

mjomble
Copy link
Contributor

@mjomble mjomble commented Apr 19, 2020

Updated the type in the table to match the extended description below

@changeset-bot
Copy link

changeset-bot bot commented Apr 19, 2020

💥 No Changeset

Latest commit: 97aac7b

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Vultraz
Copy link
Contributor

Vultraz commented Apr 19, 2020

So, interestingly, the mention of Function was deliberately removed from the table since there were various issues with Function values of defaultValue... (#2043, #2370)... except my testing seemed to indicate they were fixed in at least some cases (for non-required fields, at least). So... the current behavior definitely needs some clarification.

@MadeByMike
Copy link
Contributor

The base field class implements a getDefaultValue method that looks like this:

  getDefaultValue({ context, originalInput, actions }) {
    if (typeof this.defaultValue !== 'undefined') {
      if (typeof this.defaultValue === 'function') {
        return this.defaultValue({ context, originalInput, actions });
      } else {
        return this.defaultValue;
      }
    }
    // By default, the default value is undefined
    return undefined;
  }

This can be changed in the implementation for individual field types, but none of our fields currently change it.

This means defaultValue can really be anything. (String | Function | null) is not really accurate as many fields don't expect a string.

Copy link
Contributor

@MadeByMike MadeByMike left a comment

Choose a reason for hiding this comment

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

I attempted a wording change. Is this better or more confusing? Can we make it simpler without skipping important detail.

@MadeByMike MadeByMike merged commit 5000bb2 into keystonejs:master Apr 23, 2020
Wkasel pushed a commit to gosignal/keystone that referenced this pull request Apr 30, 2020
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