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

Get rid of executeV2, just replace execute #1980

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Conversation

benjie
Copy link
Member

@benjie benjie commented Mar 4, 2024

Description

This is a breaking change; it turns out that having both execute and executeV2 is super annoying, and the default executeV2 function that aimed to give backwards compatibility with execute was even broken... Not ideal. Rather than explaining to everyone that we have two methods, I've decided to just replace execute with executeV2 entirely. Anyone who has written a custom step class with an execute method will need to update it; but doing so is straightforward - you just need to destructure the argument and add a line of code to the top of the function that converts the new values tuple to be in the same format as the legacy values tuple:

- async execute(count: number, values: any[][], extra: ExecutionExtra) {
+ async execute({ count, values: newValues, extra }: ExecutionDetails) {
+   const values = newValues.map((dep) =>
+     dep.isBatch ? dep.entries : new Array(count).fill(dep.value)
+   );
    // REST OF YOUR FUNCTION HERE
  }

Performance impact

An improvement that is unlikely to be measurable.

Security impact

Less surface area: better security.

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

…d; please make the following change to each of your custom step classes' `execute` methods:

```diff
- async execute(count: number, values: any[][], extra: ExecutionExtra) {
+ async execute({ count, values: newValues, extra }: ExecutionDetails) {
+   const values = newValues.map((dep) =>
+     dep.isBatch ? dep.entries : new Array(count).fill(dep.value)
+   );
    // REST OF YOUR FUNCTION HERE
  }
```

For more details, see: https://err.red/gev2
Copy link

changeset-bot bot commented Mar 4, 2024

🦋 Changeset detected

Latest commit: 288fe76

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@dataplan/json Patch
@dataplan/pg Patch
grafast Patch
pgl Patch
postgraphile Patch
graphile-build-pg Patch
graphile-utils Patch
@localrepo/grafast-bench Patch
@grafserv/persisted Patch
grafserv Patch
ruru-components Patch
@localrepo/grafast-website Patch
graphile-build Patch
graphile-export Patch
graphile Patch

Not sure what this means? Click here to learn what changesets are.

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

@benjie benjie merged commit 2d19c44 into main Mar 5, 2024
24 checks passed
@benjie benjie deleted the executev2-to-execute branch March 5, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

1 participant