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.

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

Writes tests for React to show issue #49

Merged
merged 5 commits into from
Jan 22, 2018

Conversation

vincentaudebert
Copy link
Contributor

This is separating React tests from Preact tests to show an issue with connect.

console.error node_modules/react-dom/cjs/react-dom.development.js:9747
      The above error occurred in the <p> component:
          in p
          in n (created by WrapperComponent)
          in WrapperComponent

      Consider adding an error boundary to your tree to customize error handling behavior.
      Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

  ● build: default › smoke test (react) › should render

    TypeError: Cannot set property 'props' of undefined

@developit developit self-requested a review January 17, 2018 00:31
@developit developit self-assigned this Jan 17, 2018
@developit
Copy link
Owner

hiya @vincentaudebert - thanks for doing all this awesome work!

The tests look great. I'm wondering though, if the src changes are necessary given this issue @kaheglar pointed out? It's a silly error on my part that seems like the culprit here.

@vincentaudebert
Copy link
Contributor Author

vincentaudebert commented Jan 17, 2018

@developit it's up to you really for the src
I find the original version really "preacty" and I'm more comfortable with the version in this PR with a proper class. But it don't think it matters as long as the tests are passing :)
Feel free to modify it as you want.

PS: I've been checking how things are done with react-redux to rewrite src so I think it's clearly a more "reacty" way than the original one.

@developit
Copy link
Owner

Definitely agree that its more react-ey, just not sure if it adds a bunch of weight.

@developit developit merged commit f2def88 into developit:master Jan 22, 2018
@developit
Copy link
Owner

I ended up going with the (fixed) manual inheritance for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants