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

7.0.0 bug: required argument 'ABC' is missing on 'XYZ' #2894

Closed
mrtnzlml opened this issue Oct 21, 2019 · 5 comments
Closed

7.0.0 bug: required argument 'ABC' is missing on 'XYZ' #2894

mrtnzlml opened this issue Oct 21, 2019 · 5 comments

Comments

@mrtnzlml
Copy link
Contributor

Hi, I am trying to upgrade to 7.0.0 but I am getting many errors from the compiler (sample):

- Required argument 'windowDuration: String!' is missing on 'historySpending' in 'FinanceAccountDailySpendingTable_account_3Cu9cz'.
  
  Source: components/dailySpending/FinanceAccountDailySpendingTable.js (4:7)
  3:       @argumentDefinitions(interval: { type: "FinanceAccountDateTimeInterval!" }) {
  4:       historySpending(interval: $interval) {
           ^
  5:         edges {
  
  Source: components/dailySpending/FinanceAccountDailySpendingTable.js (2:5)
  1: 
  2:     fragment FinanceAccountDailySpendingTable_account on FinanceAccount
         ^
  3:       @argumentDefinitions(interval: { type: "FinanceAccountDateTimeInterval!" }) {
  
- Required argument 'currencyType: FinanceAccountCurrencyType!' is missing on 'topUpValue' in 'FinanceAccountTopUpsTable_topUps'.
  
  Source: components/account/accountTopUps/FinanceAccountTopUpsTable.js (9:11)
   8:           status
   9:           topUpValue {
                ^
  10:             ...Money_money
  
  Source: components/account/accountTopUps/FinanceAccountTopUpsTable.js (2:5)
  1: 
  2:     fragment FinanceAccountTopUpsTable_topUps on FinanceAccountTopUpConnection {
         ^
  3:       edges {

I believe this is a bug because all of these arguments are defined as required in the schema BUT they all have a default value like so:

type FinanceAccountTopUp implements Node {
  # ...
  topUpValue(currencyType: FinanceAccountCurrencyType! = BALANCE): Money
}

Therefore these arguments should not be required. Thanks for having a look guys! :)

@mrtnzlml
Copy link
Contributor Author

It reminds me this bug in GraphQL.js I reported earlier but not sure if it's related since this dependency is being removed: graphql/graphql-js#2190

@alunyov
Copy link
Contributor

alunyov commented Oct 21, 2019

@mrtnzlml thanks for reporting! I'll take a look. It's probably this line:

if (schema.isNonNull(arg.type) && !nodeArgsSet.has(arg.name)) {

@alunyov
Copy link
Contributor

alunyov commented Oct 21, 2019

Oh, while we on this topic @mrtnzlml. In the places where you're spreading FinanceAccountDailySpendingTable do you provide this $interval with @arguments?

@mrtnzlml
Copy link
Contributor Author

@alunyov Yes, that is correct. This is the fragment with spread:

fragment FinanceAccountHistoryData_account on FinanceAccount
  @argumentDefinitions(interval: { type: "FinanceAccountDateTimeInterval!" }) {
  type
  ...FinanceAccountHistoryBalancesChart_account @arguments(interval: $interval)
  ...FinanceAccountDailySpendingTable_account @arguments(interval: $interval)
}

And here is the problematic fragment:

fragment FinanceAccountDailySpendingTable_account on FinanceAccount
  @argumentDefinitions(interval: { type: "FinanceAccountDateTimeInterval!" }) {
  historySpending(interval: $interval) {
    edges {
      node {
        windowInterval {
          start
        }
        ...FinanceAccountDailySpendingRow_historySpending
      }
    }
  }
}

The is how is the field defined:

type FinanceAccount implements Node {
  historySpending(
    after: String
    before: String
    first: Int
    interval: FinanceAccountDateTimeInterval!
    last: Int

    """
    Duration uses syntax defined by ISO 8601. It is used to select two balances between which spending is calculated.
    """
    windowDuration: String! = "P1D"
  ): FinanceAccountHistorySpendingConnection
}

Are we doing it wrong? The second error example seems to be much simpler - just a fragment without any args where we use topUpValue field also without any args (they are optional).

@mrtnzlml
Copy link
Contributor Author

Hi @alunyov! I tried to fix this issue here: #2903

Could you please check it and consider releasing this fix in a patch version? It's blocking the upgrade to version 7.0.0 for us. Thank you very much! :)

mrtnzlml added a commit to adeira/relay-example that referenced this issue Nov 8, 2019
Currently blocked on this: facebook/relay#2894

adeira-source-id: 94000a04ce27fbd250c7e4dffb188b50fc9be25b
jstejada pushed a commit that referenced this issue Dec 19, 2019
Summary:
Previously, Relay Compiler threw an error incorrectly on fields with required values AND default values defined in the schema. This change takes the default values into account so that you don't have to specify them in the operation/fragment definition.

Closes: #2894
Pull Request resolved: #2903

Reviewed By: josephsavona

Differential Revision: D18109039

Pulled By: tyao1

fbshipit-source-id: c801d2eb2c3e13cc6d879ce44b6dd158f2b25aa3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants