Skip to content
This repository was archived by the owner on Dec 31, 2020. It is now read-only.

Implement new hooks proposal, as discussed in #94#130

Merged
danielkcz merged 24 commits intomasterfrom
hooks-v2
Apr 28, 2019
Merged

Implement new hooks proposal, as discussed in #94#130
danielkcz merged 24 commits intomasterfrom
hooks-v2

Conversation

@mweststrate
Copy link
Copy Markdown
Member

@mweststrate mweststrate commented Apr 15, 2019

  • make sure useLocalStore automatically converts functions to actions and binds them (and update readme example)?
  • make sure useAsObservableSource only accepts plain objects (not arrays)
  • tests
  • implementation
  • investigate if prop changes introduce double renders
  • forbid (or support) changes in the observable property set
  • update TOC
  • remove old docs
  • remove old hooks
  • fix double rendering

Update issues #129, #106, #97, #94, #38, #22 accordingly after release

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 15, 2019

Coverage Status

Coverage decreased (-0.3%) to 98.953% when pulling 17f90ba on hooks-v2 into bf51273 on master.

Copy link
Copy Markdown
Collaborator

@danielkcz danielkcz left a comment

Choose a reason for hiding this comment

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

Just a few comments. I do like where this is heading :)

@danielkcz
Copy link
Copy Markdown
Collaborator

Just want to make sure. Are you aware of linter failing? I would fix it, but I don't want to cause unnecessary merge conflicts and such. Hard to say if tests are actually passing :)

I guess double rendering cannot be really fixed after all?

FredyC and others added 2 commits April 18, 2019 12:46
@mweststrate
Copy link
Copy Markdown
Member Author

ah, I fixed those, but forgot to push indeed :)

@mweststrate
Copy link
Copy Markdown
Member Author

Fixed

@danielkcz
Copy link
Copy Markdown
Collaborator

danielkcz commented Apr 18, 2019

LGTM.

Would you mind adding simple tests to cover those error scenarios? It's an unnecessary drop in coverage imo.

https://coveralls.io/builds/22890236/source?filename=src%2FuseAsObservableSource.ts#L8

@danielkcz danielkcz changed the base branch from master to next April 28, 2019 10:12
@danielkcz danielkcz changed the base branch from next to master April 28, 2019 10:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants