Skip to content
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.

Sign up for GitHub

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

Allow the use of Promise that resolve to a state #305

Merged
merged 4 commits into from
Jul 24, 2017
Merged

Allow the use of Promise that resolve to a state #305

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9c0b46
Allow the use of Promise that resolve to a state
Swizz Jul 17, 2017
3bbae59
Move update function
Swizz Jul 21, 2017
039cca2
Rewrite update feature and add thunks support
Swizz Jul 21, 2017
af68dd2
Add some tests
Swizz Jul 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,16 @@ export function app(app) {
}).data
)

if (result != null && result.then == null) {
repaint((state = merge(state, emit("update", result))))
function update(data) {
if (data != null) {
repaint((state = merge(state, emit("update", data))))
}
}

if (result != null && typeof result.then == "function") {
result.then(update)
} else {
Copy link

@jamen jamen Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this else if (result != null) and then remove the if (result == null) above?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I think you were discussing a smaller impl above. Disregard.

update(result)
}
Copy link
Owner

@jorgebucaran jorgebucaran Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change this block to:

    if (result != null && typeof result.then === "function") {
      result.then(update)
    } else {
      update(result)
    }

and move the update implementation before the implementation of repaint right after return emit, around like 35.

Rewrite it like this please.

  function update(withState) {
    if (withState != null) {
      repaint((state = merge(state, emit("update", withState))))
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the slack discussion, we move from result.then != null to typeof result.then == "function because :

At least test if then is a function ?
If the function return nothing, the result != null will silent ignore it. But if then is not a function, that will break the thing

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Swizz That's probably a misunderstanding. If then is not a function then the user is doing something very weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we forbid the use of then in state ?

app({
  state: {
    foo: 0,
    bar: 1,
    then: 2
  },
  actions: {
    change: state => ({ then: state.then + 1 }),
  }
})

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can forbid it. I am not too worried about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we need to check if resul.then is a function in order to call it.

If the end user return a function let execute it, and the result not null check will do the rest. If this is not a function lets do a regular result patch.

Without that, using then in result will break the app with an hyperapp internal error.

Copy link
Owner

@jorgebucaran jorgebucaran Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you are saying, but yes, use:

if (result != null && typeof result.then === "function") {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use the following app

app({
  state: {
    foo: 0,
    bar: 1,
    then: 2
  },
  actions: {
    change: state => ({ then: state.then + 1 }),
  }
})

This will raise an error when using action

Uncaught TypeError: result.then is not a function

Copy link
Owner

@jorgebucaran jorgebucaran Jul 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Swizz

if (result != null && typeof result.then === "function") {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes yes 👍


return result
Expand Down