Promise in useEffect works, even though IntelliJ says 'Promise returned from Promise is ignored '

Question

I wonder how come the code below works (it displays correct options in a dropdown), even though a warning in IntelliJ says 'Promise returned from Promise is ignored' and suggests changes to the code. Initial version of code:

import React, { useState, useEffect } from "react";
import axios from "axios";

const Cake = () => {
  const [ing, setIng] = useState([]);
  const [error, setError] = useState("");

  const getIngredients = async () => {
    // returns an array of strings
    const url = new URL("someUrl");
    const result = await axios.get(url.toString());
    return result.data;
  };

  useEffect(() => {
    new Promise((resolve, reject) => {
      getIngredients()
        .then((response) => setIng(response))
        .then((response) => resolve(response))
        .catch((error) => setError(error));
    });
  }, []);

  return (
    <div>
      <Select options={ing} />
    </div>
  );
};

export default Cake;

IntelliJ suggests adding "then" to my Promise, like this:

useEffect(() => {
  new Promise((resolve, reject) => {
    getIngredients()
      .then((response) => setIng(response))
      .then((response) => resolve(response))
      .catch((error) => setError(error));
  }).then((r) => console.log(r));
}, []);

What is the purpose of wrapping your code in the `useEffect` hook in a promise? I think the issue here is that's unnecessary — Nick, Jul 27 '21 at 12:52
What's the point of wrapping `getIngredients` in a promise constructor? You already have a promise that is returned by calling `getIngredients`, no need to use the promise constructor. — Yousaf, Jul 27 '21 at 12:52
Checkout the "Creating New Promises" section of this article: https://runnable.com/blog/common-promise-anti-patterns-and-how-to-avoid-them You're creating a promise at the top of the chain where none is needed - the getIngredients() function already returns a Promise — Brendan Bond, Jul 27 '21 at 12:53
Aren't you supposed to return the `Promise` created with new? Even better, just get rid of the unnecessary wrapping Promise. `return getIngredients().then((response) => setIng(response)).catch((error) => setError(error))` See https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it — Ruan Mendes, Jul 27 '21 at 12:55
@JuanMendes `return getIngredients(...)` is not needed. A valid return value from the `useEffect` hook is a function that is used as a _cleanup_ function. — Yousaf, Jul 27 '21 at 13:04
@Yousaf That tells you how long it's been since I used `React`. I only know about `useState/Effect` from SO — Ruan Mendes, Jul 27 '21 at 14:50

score 4 · Accepted Answer · answered Jul 27 '21 at 12:58

4

You shouldn't create a new Promise here at all (which, as IntelliJ remarked, was not even used anywhere), also it's the Promise constructor antipattern to resolve a promise from the .then() callback of another promise.

All you need in your effect is

useEffect(() => {
    getIngredients().then(setIng, setError);
}, []);

answered Jul 27 '21 at 12:58

Bergi

630,263
148
957
1,375

Thank you! I have a further question concerning part inside ".then(...)": in such case catching an error is not necessary? I see that in the link you provided "unnecessary function wrappers for the .then and .catch handlers" are mentioned, but to be honest I have a problem with understanding it. Could you please explain why have you decided to omit the ".catch()" part? – Zuzu Jul 27 '21 at 13:10
I used [`.then(…, …)` instead of `.then(…).catch(…)`](https://stackoverflow.com/q/24662289/1048572) to handle errors from `getIngredients`. `setIng` is expected not to throw, if it does, that error is unhandled (same as another error thrown from `setError` would be). – Bergi Jul 27 '21 at 13:13
I think using `catch` is the preferred way instead of the second argument. If an error occurs in the first argument (`setIng`), the second argument is not called (`setError`). A catch does always get called. See https://jsfiddle.net/mendesjuan/37gpbuq8/1/ – Ruan Mendes Jul 27 '21 at 13:33
@JuanMendes "*If an error occurs in the first argument (`setIng`), the second argument is not called (`setError`).*" - and that is exactly the desirable behaviour here. We only care about errors from `getIngredients()` (and also, afaict, react state setters never throw errors). Using a `.catch()` call is not always preferred. – Bergi Jul 27 '21 at 13:36
Using the second handler requires that you make sure it doesn't throw an error, it could be an error because you tried to call a backup URL. I'm just saying it's safer, with mostly the same behavior. Why do you not want that behavior here? If an error occurs in the handler, you don't want the caller to know? It does not behave the same way as if `setError` throws an error, that will correctly propagate as an error as shown in my linked example? https://jsfiddle.net/mendesjuan/37gpbuq8/1/ – Ruan Mendes Jul 27 '21 at 13:41
@JuanMendes I don't understand what you mean by "*it could be an error because you tried to call a backup URL.*"? If an error occurs in the handler, I want to get an unhandled rejection warning in the console, because I didn't expect such an error. `setError` is only meant to handle the types of errors from `getIngredients()`, not any other errors. I very much prefer to have the guarantee that exactly one of `setIng` and `setError` are called, never both, which gives me an invariant about either `ing` or `error` being set in the component. – Bergi Jul 27 '21 at 13:50
@Bergi That was just an example of an error that could happen in a then handler. I just want people to know that `catch` and `fail` parameter behave differently and [some consider using `fail` an anti pattern](https://stackoverflow.com/questions/24662289/when-is-thensuccess-fail-considered-an-antipattern-for-promises). You have the accepted answer on that question. I'm trying to remember where I picked up the habit of always using `catch` and it just may have been that :). I know you mention it's not an anti pattern :) . TL;DR `.catch will catch errors even inside the success function` – Ruan Mendes Jul 27 '21 at 14:34

Promise in useEffect works, even though IntelliJ says 'Promise returned from Promise is ignored '

1 Answers1