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 binding to objects with more than one property #1542

Merged
merged 5 commits into from
Jan 12, 2023
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jan 11, 2023

Currently, adding a binding { a:1, b:2 } breaks the evaluator as it is interpreting this as a code block instead of an expression. This PR fixes the runtime behavior. Also opening #1543 to deal with the editor

@oliviertassinari oliviertassinari requested a deployment to fix-object-binding - toolpad-db PR #1542 January 11, 2023 13:46 — with Render Abandoned
address unreachable code
@oliviertassinari oliviertassinari temporarily deployed to fix-object-binding - toolpad PR #1542 January 11, 2023 15:19 — with Render Destroyed
@oliviertassinari oliviertassinari temporarily deployed to fix-object-binding - toolpad PR #1542 January 11, 2023 15:19 — with Render Destroyed
@Janpot Janpot marked this pull request as ready for review January 11, 2023 15:34
@Janpot Janpot enabled auto-merge (squash) January 11, 2023 15:34
window.getComputedStyle(elm).getPropertyValue('color'),
);
expect(color).toBe('rgb(25, 118, 210)');
await expect(page.getByText('-test2-')).toBeVisible();
Copy link
Member

@apedroferreira apedroferreira Jan 12, 2023

Choose a reason for hiding this comment

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

This one is testing that even if sx has a broken binding like { the element still shows?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, and that the app doesn't crash

@Janpot Janpot merged commit 8edd2e9 into master Jan 12, 2023
@Janpot Janpot deleted the fix-object-binding branch January 12, 2023 15:47
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 13, 2023

Using a bisection, I have found this commit breaking the support for:

function run() {
  return "Not found"
}
run()
real use case
function run() {
  if (getRepositoryDetails.data) {
    return `Repository ID: ${getRepositoryDetails.data.id}`
  } else {
    return "Not found"
  }
}
run()

and

const nonProductScopeLabels = ["support: commercial"]

nonProductScopeLabels
real use case
const nonProductScopeLabels = ["support: commercial"]

materialUI.rows
  .concat(muix.rows)
  .filter((issue) => {
    const withoutNonProductScopeLabels = issue.labels.filter(
      (label) => !nonProductScopeLabels.includes(label.name)
    )
    return withoutNonProductScopeLabels.length === 1
  })
  .sort((issueA, issueB) => {
    if (issueA.state === "open") {
      return 1
    }
    if (issueB.state === "open") {
      return 1
    }
    return issueB.number - issueA.number
  })

in the binding. This was reported by Greg because https://master--toolpad.mui.com/_toolpad/app/cl6rqzry10009arlv9sto6qja/pages/ip23ggo throws

Screenshot 2023-01-13 at 23 07 38

(my previous link gets redirected to a wrong URL but this is a different bug)

@Janpot
Copy link
Member Author

Janpot commented Jan 13, 2023

🤔 hmm, this was always intended to only support javascript expressions. i.e. anything that can go the the right side of an assigment. Otherwise it gets a bit ambiguous to define which kind of code it accepts exactly. But you should be able to

(() => {
  if (getRepositoryDetails.data) {
    return `Repository ID: ${getRepositoryDetails.data.id}`
  } else {
    return "Not found"
  }
})()

and

(() => {
  const nonProductScopeLabels = ["support: commercial"]

  return materialUI.rows
    .concat(muix.rows)
    .filter((issue) => {
      const withoutNonProductScopeLabels = issue.labels.filter(
        (label) => !nonProductScopeLabels.includes(label.name)
      )
      return withoutNonProductScopeLabels.length === 1
    })
    .sort((issueA, issueB) => {
      if (issueA.state === "open") {
        return 1
      }
      if (issueB.state === "open") {
        return 1
      }
      return issueB.number - issueA.number
    })
})()

Otherwise we need to take it back to the drawing board

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 14, 2023

@Janpot Alright, thanks. (() => { I'm using this fix now, we (Greg, for the KPIs page, etc.) need the app to work until we decide what's the right solution (if it's not this one).

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