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: do validation for params off constructor so it can throw before starting #1984

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Nov 16, 2024

did this wrong in #1978 so that combined with #1983, passing a reserved parameter just means it silently fails atm.

Copy link
Contributor

@samwillis samwillis left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines 538 to 545
const reservedParams = Object.keys(options.params).filter(
(key) => RESERVED_PARAMS.has(key)
)
if (reservedParams.length > 0) {
throw new Error(
`Cannot use reserved Electric parameter names in custom params: ${reservedParams.join(`, `)}`
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this could be simpler?

for (const param of RESERVED_PARAMS) {
  if (param in options.params) {
    throw new Error(`Cannot use reserved Electric parameter name in custom params: ${param}`);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing test checks all parameters before throwing an error — so the error message is more comprehensive than if you throw immediately.

Copy link

netlify bot commented Nov 18, 2024

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit 22c1dcf
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/673bac00d4180200082883ad
😎 Deploy Preview https://deploy-preview-1984--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@KyleAMathews KyleAMathews merged commit 91e5b85 into main Nov 18, 2024
16 of 18 checks passed
@KyleAMathews KyleAMathews deleted the fix-validation branch November 18, 2024 21:27
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.

2 participants