`refreshApex` errors if the response is undefined

[rgalanakis]

Description

This code: refreshApex({data: undefined, error: undefined}) will return undefined rather than a promise. It should return a promise (resolved or rejected, either seems reasonable) to adhere to async conventions.

Steps to Reproduce

Call refreshApex({data: undefined, error: undefined}).then(() => console.log('x')), observe TypeError Cannot read property 'then' of undefined.

Expected Results

No error is thrown (refreshApex returns a promise).

Actual Results

refreshApex returns undefined.

Browsers Affected

All.

Version

Current Salesforce.

Possible Solution

Return Promise.resolve() instead, since this seems like a noop.

[git2gus[bot]]

This issue has been linked to a new work item: W-8789919

[mburr-salesforce]

@rgalanakis We're looking at the best way to correct this, but out of curiosity was the { data: undefined, error: undefined } the exact object that your Apex @wire had emitted (which we should support) or just a constant you were passing to refreshApex (not supported)?

[rgalanakis]

We have something like this:

    @wire(getRecord, { recordId: "$communicationId", fields: commFields.schemas() })
    wiredRecord(result) {
        this._getRecordResponse = result;
        const { data, error } = result;
        // more code
    }

    afterSomeThing() {
        return refreshApex(this._getRecordResponse);
    }

When I console logged this._getRecordResponse, I got { data: undefined, error: undefined }. I believe this is because it's the initial object done in the wire callback before data has loaded, and in this case, we weren't passing in @api communicationId so it never re-ran/had real data.

[mburr-salesforce]

refreshApex is only meant to refresh values obtained from an Apex @wire. Using it to refresh data from other wire adapters is unsupported and is not guaranteed to continue working in future releases. Take a look at getRecordNotifyChange if you need to let us know that the record data we have cached is stale.

[rohanshenoy96]

I'm still getting error on a wired apex call. Not sure what I did wrong

@wire(getBoats, { boatTypeId: '$boatTypeId' })
    wiredBoats({data, error}) {
        if (data) {
            // console.log(data);
            this.boats = data;
            this.isLoading = false;
            // this.dispatchEvent(new CustomEvent('isdoneloading', {detail: true}));
        } else {
            console.log(error);
            this.boats = undefined;
            this.isLoading = false;

            // this.dispatchEvent(new CustomEvent('isdoneloading', {detail: true}));
        }
    }

async handleSave(event) {
        console.log(JSON.stringify(event.detail));
        const updatedFields = event.detail.draftValues;

        // List if record Ids
        const notifyChangeIds = updatedFields.map(row => ({'recordId': row.Id}));

        await updateBoatList({data: updatedFields})
        .then(results => {
            const toast = new ShowToastEvent({
                title: SUCCESS_TITLE, 
                message: MESSAGE_SHIP_IT, 
                variant: SUCCESS_VARIANT 
            });
            this.dispatchEvent(toast);

            getRecordNotifyChange(notifyChangeIds);
            return refreshApex(this.boats).then(() => {
                // Clear all draft values in the datatable
                this.draftValues = [];
            });
        })
        .catch(error => {
            console.log(error);
            const toast = new ShowToastEvent({
                title: ERROR_TITLE, 
                message: error.body.message, 
                variant: ERROR_VARIANT 
            });
            this.dispatchEvent(toast);
        });
    }

[rgalanakis]

@mburr-salesforce

refreshApex is only meant to refresh values obtained from an Apex @wire. Using it to refresh data from other wire adapters is unsupported and is not guaranteed to continue working in future releases. Take a look at getRecordNotifyChange if you need to let us know that the record data we have cached is stale.

I'm confused (also I must have missed your followup, my bad)- the question isn't about the 'business logic' behavior, this is about the "type" behavior. There is no circumstance I can think of that I'd want a return type value of undefined|Promise<any>. Especially if the return value type is based on the value of the input (and not the type or shape of the input, which I still think would be an unexpected behavior, but could imagine a justification).

Consider for example the NodeJS fs module which works like every other Promise-based API I've seen:

> const fs = require('fs/promises');
undefined
> fs.stat('xyz').catch((e) => console.log(e))
Promise { <pending> }
> [Error: ENOENT: no such file or directory, stat 'xyz'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'stat',
  path: 'xyz'
}
> fs.stat(1).catch((e) => console.log(e))
Promise { <pending> }
> TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received type number (1)
    at Object.stat (internal/fs/promises.js:518:10)
    ... {
  code: 'ERR_INVALID_ARG_TYPE'
}

> fs.stat(undefined).catch((e) => console.log(e))
Promise { <pending> }
> TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received undefined
    at Object.stat (internal/fs/promises.js:518:10)
    .... {
  code: 'ERR_INVALID_ARG_TYPE'
}

That is, it always returns a Promise, even with an invalid argument. What I see in refreshApex is instead the equivalent of:

> function maybeAsync(x) { return x ? Promise.resolve(1) : undefined }
undefined
> maybeAsync(5).then((x) => console.log(x))
Promise { <pending> }
> 1
> maybeAsync(0).then((x) => console.log(x))
Uncaught TypeError: Cannot read property 'then' of undefined

That is very difficult to program against- it doesn't really make sense to return undefined|Promise<any>. It should always be Promise<any>. This seems like an oversight, not an intentional design decision. If you need some examples of why this pattern is painful to program against (which, again, is why you never really see it in the wild), I can provide.

Hopefully that better describes the expected result.

[mburr-salesforce]

@rohanshenoy96 A few comments on the code you pasted.

@wire(getBoats, { boatTypeId: '$boatTypeId' })
    wiredBoats({data, error}) {

If you want to call refreshApex you'll need to save the parameter to wiredBoats without destructuring it. That is, something like:

@wire(getBoats, { boatTypeId: '$boatTypeId' })
    wiredBoats(result) {
        this.wireBoatsResult = result;
        if (result.data) {
        ...

then pass that value to refreshApex:

            return refreshApex(this.wireBoatsResult).then(() => {

I'm also guessing that:

await updateBoatList({data: updatedFields})
        .then(results => {

should probably not have the await, e.g.:

updateBoatList({data: updatedFields}).then(results => {

And I'm not sure what the record ids that you're passing to getRecordNotifyChange are or where they come from? The code is not necessarily wrong, but from just what you have pasted above the call to getRecordNotifyChange will not have any effect since you don't have any @wires watching those records.

[rohanshenoy96]

Thanks @mburr-salesforce. You're right I tried it without destructuring it and it totally worked. I don't understand why this restriction is there as many might get stuck here. Also even I felt using await was weird since we were using then and catch(got it from somewhere probably stackexchange). Removed await and it works. Thanks a bunch

[mburr-salesforce]

@rgalanakis

That is very difficult to program against- it doesn't really make sense to return undefined|Promise. It should always be Promise.

You are correct, and this is the intended behavior of refreshApex when it is used correctly. The behavior you see is the confluence of a couple things:

1. There is the bug you noticed and reported (thank you!) when a caller passes the initial { data: undefined, error: undefined } value from an Apex @wire to refreshApex. This has been fixed in our code to return Promise.resolve(undefined), but the change is unlikely to make it into the Spring 21 release due to timing.
2. The API documentation does not define the behavior when the caller passes a value that was not previously emitted from an Apex @wire. The current implementation will return undefined when run in production mode and throw an error when run in other modes. As you correctly note above, returning Promise.reject(...) would be the cleanest solution, but since this code has been shipped for several releases there is a risk that some customers are relying on the current behavior, either inadvertently or intentionally. Rather than risk breaking those customers we have opted to preserve the current suboptimal behavior for this error scenario and update the documentation to provide more details on the correct usage (this is still in progress).

So tl;dr - once the fix for (1) goes live, refreshApex should always return a Promise when the value you pass it was previously emitted by an Apex @wire; you should only get undefined back if you are passing a bad parameter.

[rgalanakis]

@mburr-salesforce

you should only get undefined back if you are passing a bad parameter.

I'd encourage you to revisit this choice (even though it isn't directly what we hit). Consider this:

> // I'm assuming if `wireResult is the empty result, it will just resolve, if it rejects, that's fine too
> refreshApex(this.wireResult).then((r) => console.log(r)).catch((e) => console.error(e))
Promise { <pending> }
undefined
> refreshApex(null).then((r) => console.log(r)).catch((e) => console.error(e))
Uncaught TypeError: Cannot read property 'then' of undefined

Compare to something like:

> refreshApex(null).then((r) => console.log(r)).catch((e) => console.error(e))
Promise { <pending> }
error: TypeError: refreshApex argument must be the result of a wire call. See <url explaining error>. Received: null

Returning undefined will lead to a TypeError unless the user inspects the source of the code and/or does a search to debug what the issue is (probably leading here!). You miss the opportunity to explain the error if the code doesn't always return a promise.

Alternatively you could do something like this, and raise an error explicitly, rather than return a promise:

> refreshApex(null).then((r) => console.log(r)).catch((e) => console.error(e))
TypeError: refreshApex argument must be the result of a wire call. See <url explaining error>. Received: null

But IMO the rejected promise is nicer. In any case though, just returning undefined is guaranteed to be a poor user experience.

[mburr-salesforce]

Alternatively you could do something like this, and raise an error explicitly, rather than return a promise:

If you turn on debug mode this is exactly what happens. The error message is Error: Refresh failed because resolved configuration is not available.. As a general rule, though, Lightning code is written to fail silently for non-critical errors when running in production mode.

But IMO the rejected promise is nicer. In any case though, just returning undefined is guaranteed to be a poor user experience.

I understand and agree with you, and if we were discussing new code that had never shipped to customers I would already be changing it to return a rejected promise. The challenge here is that this code has shipped to customers, some of whom might already have assumptions about the current behavior baked into their code. I am a huge fan of consistent APIs, but I have to balance my desire to have this error path behave more correctly in production mode against the (admittedly small) risk of costing some company millions of dollars when I break a business-critical function for a week in their 10k user org.

[cwarden]

But IMO the rejected promise is nicer. In any case though, just returning undefined is guaranteed to be a poor user experience.

I understand and agree with you, and if we were discussing new code that had never shipped to customers I would already be changing it to return a rejected promise. The challenge here is that this code has shipped to customers, some of whom might already have assumptions about the current behavior baked into their code. I am a huge fan of consistent APIs, but I have to balance my desire to have this error path behave more correctly in production mode against the (admittedly small) risk of costing some company millions of dollars when I break a business-critical function for a week in their 10k user org.

Maybe it can be a versioned fix in a future API version?

[mburr-salesforce]

@cwarden that's certainly a possibility if/when we get the component's apiVersion plumbed far enough down into the code.

[rgalanakis]

The challenge here is that this code has shipped to customers, some of whom might already have assumptions about the current behavior baked into their code.

Fair enough- I think the situation we're discussing is very close to "is not guaranteed to continue working in future releases" but not my call.

I will mention one last thing though (and then I'll try to stop being such a stubborn thorn) that my concern is that the "return undefined" in this case is less likely to lead to "fail silently for non-critical errors when running in production mode." The Aura library may fail silently but it's very likely to lead to a very loud error running in production mode in the caller code. Or put differently, I think "fail silently for non-critical errors when running in production mode" is a reasonable approach, but "return different types when running in production mode" is the wrong choice.