feat(wire-service): update wire method arg to an object #44
Conversation
Benchmark comparisonBase commit:
|
| this.cmp[this.propName](null, value); | ||
| // TODO: deprecate (error, data) args | ||
| if (wireMethod.length === 2) { | ||
| wireMethod.call(this.cmp, null, value); |
There was a problem hiding this comment.
should we log a warning a deprecation warning the console here?
There was a problem hiding this comment.
sure, however this is going to be short-lived (2 weeks)
| this.cmp[this.propName](err, undefined); | ||
| // TODO: deprecate (error, data) args | ||
| if (wireMethod.length === 2) { | ||
| wireMethod.call(this.cmp, err, undefined); |
There was a problem hiding this comment.
should we log a warning a deprecation warning the console here?
Benchmark comparisonBase commit:
|
| console.warn('[DEPRECATE] @wire function no longer supports two arguments (error, data), please update your code to use ({error, data}) instead.'); | ||
| wireMethod.call(this.cmp, err, undefined); | ||
| } else { | ||
| wireMethod.call(this.cmp, { data: undefined, error: err }); |
There was a problem hiding this comment.
Whats the reasoning behind using undefined for missing data and null for missing error? Do we even need to include data and error keys at all in these cases?
There was a problem hiding this comment.
interesting question, but not really about this PR. @yungcheng can you open an issue to discuss that?
I have no strong opinions here. In general I think of errors as objects, in which case it could be null or an object that detonates the error, while I think of data as anything that might or might not exists, in which case undefined is more accurate.
It will be interesting to mix this with the fact that the request might be in-flight during the first rendering process, and if that's the case, what will be the guideline for people to protect against that situation? /cc @kevinv11n
There was a problem hiding this comment.
@kevinv11n wanna chime in?
There was a problem hiding this comment.
the request might be in-flight
I don't understand the significance of that. Can you elaborate?
When it was a function call with 2 arguments we had to provide a value for the first argument. I spec'ed it as undefined. Now that we're using an object I think we shouldn't define the key when it has no value. So this line becomes wireMethod.call(this.cmp, { error: err })
And line 125: wireMethod.call(this.cmp, { data: value })
With destructuring, this results in undefined being assigned to the absent keys.
| console.warn('[DEPRECATE] @wire function no longer supports two arguments (error, data), please update your code to use ({error, data}) instead.'); | ||
| wireMethod.call(this.cmp, err, undefined); | ||
| } else { | ||
| wireMethod.call(this.cmp, { data: undefined, error: err }); |
There was a problem hiding this comment.
the request might be in-flight
I don't understand the significance of that. Can you elaborate?
When it was a function call with 2 arguments we had to provide a value for the first argument. I spec'ed it as undefined. Now that we're using an object I think we shouldn't define the key when it has no value. So this line becomes wireMethod.call(this.cmp, { error: err })
And line 125: wireMethod.call(this.cmp, { data: value })
With destructuring, this results in undefined being assigned to the absent keys.
Details
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path for existing applications:
Wired method argument has been updated to
{error, data}instead oferror, data. However, it's a phasing approach so if thewiredMethod.length === 2, the old approach is still supported