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

Conversation

@tkh44
Copy link
Contributor

@tkh44 tkh44 commented Apr 6, 2017

This is a good start towards getting perf up. I'm still having some issues on startup when tons of elements are created, but it is much better than before.

#348

src/index.js Outdated

if (typeof componentClass !== 'function') return;
let name = (
componentClass.prototype.displayName
Copy link
Member

Choose a reason for hiding this comment

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

this seems odd - was componentClass.displayName working before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests would fail. It was returning the function you use to construct the pfcs. This was before I was testing on 8. I'll adjust and try again.

- moved the validation call into createElement/cloneElement to match React
- added test for cloneElement

preactjs#348
@tkh44 tkh44 changed the title Initial work for #348 Only run propType checks on createElement & cloneElement not on every render. Apr 7, 2017
@tkh44
Copy link
Contributor Author

tkh44 commented Apr 7, 2017

All the tests pass, but there are missing tests for children as function. I tried to get this covered, but the tests would run as expected in the browser but not in phantom. The proptype checking for children as a function (I think any "single child" case) is proving to be difficult.

@tkh44
Copy link
Contributor Author

tkh44 commented Apr 7, 2017

The good news is that even with this issue the performance difference is remarkable.

Before (pink is proptype checks)
After (Red frames due to error throwing)

@developit
Copy link
Member

Looking good - will test this out locally and see if I can make a children as function test work.

@tkh44
Copy link
Contributor Author

tkh44 commented Apr 15, 2017

Rebased after #359 landed.
This was a crazy rebase so I might have messed it up (commit order). Let me know if I need to change or update anything.

@developit
Copy link
Member

developit commented Apr 16, 2017

Odd that it's still noting merge conflicts in GH's UI.

@tkh44
Copy link
Contributor Author

tkh44 commented Apr 17, 2017

I will close this PR and and redo it.

@tkh44 tkh44 closed this Apr 25, 2017
@tkh44
Copy link
Contributor Author

tkh44 commented Apr 25, 2017

New PR opened on #370

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.

3 participants