-
Notifications
You must be signed in to change notification settings - Fork 393
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(engine-core): remove dev-only errors for decorators #3409
Conversation
Partially addresses #3245
assert.invariant( | ||
isFunction(get), | ||
`Invalid compiler output for public accessor ${toString(key)} decorated with @api` | ||
); |
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.
If you look at the logic above, this is doing the same thing. It was throwing in both dev mode and prod mode.
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.
Can you please update the error message to say "the property is missing a getter."? It will improve the DX.
Currently the message is abstract because it does not exactly say what is wrong, it also seems to indicate that the compiler output is incorrect. If the user does not look at the dev console, they might miss the cause.
@@ -81,17 +80,13 @@ function validateObservedField( | |||
fieldName: string, | |||
descriptor: PropertyDescriptor | undefined | |||
) { | |||
assertNotProd(); // this method should never leak to prod |
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.
Added assertNotProd
s for my own sanity
// This line of code does not seem possible to reach. Maybe when native decorators are supported? | ||
logError( | ||
`Invalid @wire ${methodName} field. The field should have a valid writable descriptor.` | ||
); |
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.
A bunch of these validations seem impossible to hit. (I tried.) So I just noted it in the comment.
The main reason they can't be hit is that decorators append the registerDecorators()
call right after the class
declaration, so there's no chance to do any funny business with Object.defineProperty()
, because that would occur after the registerDecorators()
.
I guess someone could do funny business after registerDecorators
, but if someone's doing that, then they're already broken today.
logError( | ||
`@wire on method "${fieldOrMethodName}": adapter id must be truthy.` | ||
); | ||
} |
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.
Despite the name, assert.isTrue()
actually asserts truthiness, not === true
. So this logic is the same as before.
/nucleus ignore --reason 'lds-lightning-platform is not passing right now' |
/nucleus test |
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.
LGTM
@@ -155,15 +152,20 @@ function validateAccessorDecoratedWithApi( | |||
fieldName: string, | |||
descriptor: PropertyDescriptor | undefined | |||
) { | |||
assertNotProd(); // this method should never leak to prod | |||
if (isUndefined(descriptor)) { |
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.
registerDecorators()
which is the only caller of this function already checks if descriptor
is undefined and it throws. However it does it after invoking this function. You can switch the order there and eliminate this undefined check.
assert.invariant( | ||
isFunction(get), | ||
`Invalid compiler output for public accessor ${toString(key)} decorated with @api` | ||
); |
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.
Can you please update the error message to say "the property is missing a getter."? It will improve the DX.
Currently the message is abstract because it does not exactly say what is wrong, it also seems to indicate that the compiler output is incorrect. If the user does not look at the dev console, they might miss the cause.
/nucleus test |
/nucleus test |
/nucleus ignore --reason 'lwc-platform needs updating' |
Details
Partially addresses #3245
Anytime we throw an error in dev mode but not in prod mode, that's a bug. This PR gets us one step closer to fixing that, by making decorator validation errors log rather than throw.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?