Should manual request return promise?

[ashtuchkin]

When doing requests from event handlers, I wanted to be notified when request finishes, so that I could show message to the user. Like this:

onSave() {
  this.props.updateData(this.state.data).then(() => flash("Saved"))
}

I'm not sure what value should be passed to the promise though.

[hnordt]

You can read from PromiseState. It returns fulfiled when finished.

[ryanbrainard]

@ashtuchkin it will inject a PromiseState to whatever prop you assigned in updateData() inside of connect(). If you want to do something with that like show a user message, you'll need to check that it is both defined and fulfilled. It will be undefined until you call updateData(). Use componentWillReceiveProps() if you want to compare it to the current value, otherwise just use it in render().

[ashtuchkin]

I guess I can do that. Maybe we can do better though.. See my case of a simple edit page with a save button:

@connect((props) => ({
    project: `/api/projects/${props.id}`,
    updateProject: (data) => ({
        project: { url: `/api/projects/${props.id}`, method: "PATCH", body: data }
    })
}))
class ProjectEditPage extends React.Component {
    constructor(props) {
        super()
        // We don't have the data here as it's not loaded.
        this.state = {}
    }
    componentWillReceiveProps(props) {
        if (props.project.fulfilled) {
            // When the project is loaded or updated, we set the state to new values.
            this.setState({props.project.value})

            // How do we notify user that save is successful? Is this even a good place?
        }
    }

    // 200 lines here

    saveProject() {
        this.props.updateProject(this.state)
        // isn't it a lot clearer to just add .then(() => flash("Save successful")) here?
    }
    render() {
        if (this.props.project.pending)
            return <Loading/>
        if (this.props.project.rejected)
            return <Error ... />

        return <div>
             Project name: <input value={this.state.name} onChange={..change this.state.name}/>
             ....other props...
             <button onClick={()=>this.saveProject()}>Save project</button>
        </div>
    }
}

[hnordt]

@ashtuchkin

You don't need to use state, you have two options:

class ProjectEditPage extends React.Component {
    saveProject() {
      if (props.project.fulfilled) {
        this.props.updateProject(props.project.value);
      }
    }
    render() {
        if (this.props.project.pending)
            return <Loading/>
        if (this.props.project.rejected)
            return <Error ... />

        return <div>
             Project name: <input value={this.state.name} onChange={..change this.state.name}/>
             ....other props...
             <button onClick={()=>this.saveProject()}>Save project</button>
        </div>
    }
}

Or:

class ProjectEditPage extends React.Component {
    saveProject() {
      this.props.updateProject(props.project.value);
    }
    render() {
        if (this.props.project.pending)
            return <Loading/>
        if (this.props.project.rejected)
            return <Error ... />

        return <div>
             Project name: <input value={this.state.name} onChange={..change this.state.name}/>
             ....other props...
             {props.project.fulfilled && <button onClick={()=>this.saveProject()}>Save project</button>}
        </div>
    }
}
```jsx

[ashtuchkin]

@hnordt not following you.. I need user to edit these inputs. those values are definitely state. Plus, where do I notify user that the save is successful?

[hnordt]

@ashtuchkin

Here is my idea:

@connect(({ id }) => ({
  project: `/api/projects/${id}`,
  updateProject: data => ({
    project: { url: `/api/projects/${id}`, method: 'PATCH', body: data }
  })
}))
class ProjectEditPage extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      project: null,
      projectUpdated: null
    };
  }
  componentWillReceiveProps(nextProps) {
    if (nextProps.project.fulfilled) {
      this.setState({
        project: nextProps.project.value,
        projectUpdated: this.state.project.isDirty // if project was dirty, means it was updated
      });
    }
  }
  handleProjectChange(property, value) {
    this.setState({
      project: Object.assign({}, this.state.project, {
        [property]: value,
        isDirty: true
      })
    })
  }
  render() {
    const {
      project: {
        pending: isPending,
        rejected: isRejected
      },
      updateProject
    } = this.props;
    if (isPending) {
      return <Spinner />;
    }
    if (isRejected) {
      return <Error />;
    }
    const {
      project,
      projectUpdated
    } = this.state;
    return (
      <div>
        {projectUpdated && <strong>Updated!</strong>}
        {project && (
          <div>
            Project name:
            <br />
            <input value={project.name} onChange={event => this.handleProjectChange('name', event.target.value)} />
            <button onClick={() => updateProject(project)}>Save project</button>
          </div>
        )}
      </div>
    );
  }
}

[ashtuchkin]

So basically you propose to add an isDirty flag to state and check it in componentWillReceiveProps. This will work, although it seems more like a workaround to me.

I still think returning a promise makes sense conceptually (as a signal that the action is done) and makes code cleaner/easier to follow. I do understand that extending api surface will require more maintenance though, so will not press. Thanks.

[hnordt]

@ashtuchkin am I wrong or we can make it work using PromiseState?

PromiseState.all([project]).then(() => flash())

Have you also tried this.props.project.then()?

[ashtuchkin]

Yeah, but it's has different semantics. this.props.project.then() is data-specific (transform the data when it's loaded), but I wanted something action-specific (that a particular action is finished).

[hnordt]

@ryanbrainard if we have this.props.project.then() why not this.props.saveProject().then()? :P

I agree with @ashtuchkin, returning a promise is helpful.

[ryanbrainard]

Sorry for the delay. I didn't see these comments because they were on a closed issue. Although it wasn't really designed for it, you could try to use andThen() on the request:

@connect((props) => ({
    project: `/api/projects/${props.id}`,
    updateProject: (data) => ({
        project: { 
          url: `/api/projects/${props.id}`, 
          method: "PATCH", 
          body: data,
          andThen: () => {
            // do something here
            return {}
          }
        }
    })
}))

This is kind of crappy because now there is a side effect, but since it is inside of a fetch function, I guess it is less bad. The other problem is that you probably don't have access to whatever you want to call here either. If we add "identity requests", they could actually work quite well here too, so you could do something like this:

@connect((props) => ({
    project: `/api/projects/${props.id}`,
    updateProject: (data) => ({
        project: { 
          url: `/api/projects/${props.id}`, 
          method: "PATCH", 
          body: data,
          andThen: () => ({
            projectUpdated: { 
              value: true // some static value of your choice
            }
          })
        }
    })
}))

Then you'd get projectUpdated in the component as a PromiseState with the provided value. This would let you know that the project is new and a result of calling updateProject(). Not sure if this gives you everything you need, but I think that is at least a step in the right direction. Let me know your thoughts.

Going to re-open this so I see the comments.

[ryanbrainard]

Closing this to be implemented at identity requests in #33. Re-open if there is more discussion to be had.