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

[feature] simplify codebase to expose internal API #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pgarciacamou
Copy link
Owner

@pgarciacamou pgarciacamou commented Oct 10, 2018

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

Short description of what this PR does:

  • The main reason for this update is to expose createContextConsumerChain to allow using this library not only as HOC but also to render inside a component.
  • The codebase is modularized and simplified
  • More tests added for each new module

@pgarciacamou pgarciacamou added the enhancement New feature or request label Oct 10, 2018
@pgarciacamou pgarciacamou self-assigned this Oct 10, 2018
@pgarciacamou
Copy link
Owner Author

pgarciacamou commented Oct 10, 2018

Work in progress label added.


I have to do some heavy profiling before this PR is merged to determine if the new implementation is actually better or worse.

The reason is that I refactored the consumeContext to be able to remove all the React.fowardRef leaving only 1.

That said, all tests pass, i.e. the behavior is exactly the same, the only difference is that there is an extra function that is created when the consumer chain executes and that could impact rendering time even if the rest of the forwardRef were removed.

@pgarciacamou pgarciacamou force-pushed the feature/simplify-codebase branch from 0b21ceb to 3ec417c Compare October 10, 2018 22:46
@pgarciacamou pgarciacamou added the work in progress Something is still pending label Oct 10, 2018
@pgarciacamou
Copy link
Owner Author

pgarciacamou commented Sep 15, 2019

After a year, I still think this is a valid idea.

This PR is something I would like to revisit now that I'm working on version 3.

- Expose inner functionality `createContextConsumerChain`
### Fixed
### Changed
- Simplify codebase by removing all but one forwardRef
Copy link
Owner Author

Choose a reason for hiding this comment

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

definitely not a minor version bump

@pgarciacamou pgarciacamou added the question Further information is requested label Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested work in progress Something is still pending
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant