Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.Sign up
Code review #4
I made the review as a pull request that contains all of the package's code, over at thibaudcolas#1, so it would be easier to relate comments to code. But I also summarised most of the comments below, so they are easier to relate to one another, and prioritise. It's probably easier to first read this list, then the comments in the PR.
These are the code-level problems I would expect to cause the most pain / actual bugs. They are ordered by how important I think they are to address sooner rather than later.
Generally I haven't seen much error handling code. I would expect the inner script execution to be the most problematic, since it will be very common for third-party code to break.
Minor but still sources of concern
Packaging - build & dependenceies
The general problem here is that the library is compiled as if it was an app, instead of a library, with all of its dependencies bundled instead of resolved by npm on install.
To me this is what would be the most worthy of documentation. The
Development & demo env
I'm sure the current StreamField implementation isn't particularly SR-friendly, so we're not aiming super high, but there are obvious improvements to be made here.
react-redux recently released its v6, which uses the new React context API from React 16.4, and introduces a change in behavior that affects this package:
This is the problematic code:
The consequence is that
I can see a few solutions:
Anyway, it's not necessary to upgrade to v6 now. I also have two concerns with the upgrade:
Finally on the react-redux front, I'm surprised that all/most of the components in the package are connected. I would expect the performance to be better if some of the connections were removed, as they clearly duplicate their computation (but use
This would also make it easier to write tests for those components, which I would really like to see.
Smaller JS / React things