-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add NonExposed parameter decorator #1395
Add NonExposed parameter decorator #1395
Conversation
PR Summary
|
packages/framework-integration-tests/src/commands/change-cart-item.ts
Outdated
Show resolved
Hide resolved
21997ab
to
29c97d0
Compare
/integration sha=eb84eb9 |
⌛ Integration tests are running... Check their status here 👈 |
/integration sha=b079792 |
⌛ Integration tests are running... Check their status here 👈 |
❌ Oh no! Integration tests have failed |
✅ Integration tests have finished successfully! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. I just left two code improvements
fields: Array<PropertyMetadata>, | ||
excludeProps?: Array<string> | ||
): Array<PropertyMetadata> { | ||
return excludeProps ? fields.filter((field: PropertyMetadata) => !excludeProps.includes(field.name)) : [...fields] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: We can just return the same array instead of making a copy.
return excludeProps ? fields.filter((field: PropertyMetadata) => !excludeProps.includes(field.name)) : [...fields] | |
return excludeProps ? fields.filter((field: PropertyMetadata) => !excludeProps.includes(field.name)) : fields |
return methodName | ||
} | ||
const argumentNames = getFunctionArguments(target) | ||
return argumentNames[parameterIndex!] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using !
will throw if the value is undefined. I know that, supposedly, if methodName
is undefined, then parameterIndex
must not... but we can't be sure. My suggestion is to add a explicit throw here with a message that tells exactly that "we could not get field name information in {this class}, {this method name}, etc.".
That way, we would know instantly what the error is about, instead of needing to debug something like undefined can be used to index
or, worse, fail silently and having an array of "non exposed fields" all filled with undefined 😂
/integration sha=bdf4b0a |
⌛ Integration tests are running... Check their status here 👈 |
✅ Integration tests have finished successfully! |
Description
Add a decorator to remove a parameter from GraphQL Schema
Changes
Checks