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

Fix issue with reserved prop names inside firebaseConnect #613

Merged
merged 9 commits into from
Jan 16, 2019

Conversation

theashguy
Copy link
Contributor

Description

Prior to this commit firebaseConnect used the prop 'firebase' to receive the firebase app reference. This meant that if another prop called firebase (for instance, from the default reducer configuration) was supplied to a firebaseConnect wrapped component it would clobber the variable. This would result in either unexpected data in the store, or alternatively the inability for developers to nest firebaseConnect wrapped components.

An example case by @iamthefox has been uploaded here:
https://github.com/iamthefox/react-redux-firebase/blob/next-nested-issues/examples/complete/nested-issue/src/Home.js#L30

This commit renames the internal reference, and also checks to see if reserved words are passed through and throws an error with a useful message that can help developers debug if they accidentally provide one of these reserved props.

Note: This same issue could exist for firestore as well, however I don't have any examples to test with.

Check List

If not relevant to pull request, check off as complete

  • [ x ] All tests passing
  • [ x ] Docs updated with any changes or examples if applicable
  • [ x ] Added tests to ensure new feature(s) work properly

Relevant Issues

Raised in gitter, example provided in description.

Prior to this commit firbaseConnect used the prop 'firebase' to
receive the firebase app reference. This meant that if another
prop called firebase (for instance, from the default reducer
configuration) was supplied to a firebaseConnect wrapped
component it would clobber the variable. This would result in
either unexpected data in state, or alternatively the inability
for developers to nest firebaseConnect wrapped components.

This commit renames the internal reference, and also checks
to see if reserved words are passed through and throws an error
with a useful message that can help developers debug if they
accedentally provide one of these reserved props.
@theashguy
Copy link
Contributor Author

Hrmm. Just got off a plane, will look at cleaning this up tomorrow 👍

@codecov
Copy link

codecov bot commented Jan 11, 2019

Codecov Report

Merging #613 into next will increase coverage by 0.2%.
The diff coverage is 33.33%.

@@           Coverage Diff            @@
##            next     #613     +/-   ##
========================================
+ Coverage   78.4%   78.61%   +0.2%     
========================================
  Files         26       26             
  Lines        917      926      +9     
  Branches     165      166      +1     
========================================
+ Hits         719      728      +9     
  Misses       198      198

@theashguy
Copy link
Contributor Author

Fixed @prescottprue, have a look and let me know if you want anything changed and / or a different approach entirely. Tried to make it resilient to this class of errors in the future.

Unfortunately we can't test for what the developer is going to pass into a connected component in build (in fact in some cases the developer might not even know off the top of their head), but we can prevent clashes during execution and raise a helpful error, plus make the internals obscure enough that they wont get hit.

@prescottprue
Copy link
Owner

Really awesome to see the fix and such a comprehensive PR/explanation around it, thanks! I'll merge after testing it a bit

Copy link
Owner

@prescottprue prescottprue 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 small typo

src/firebaseConnect.js Outdated Show resolved Hide resolved
src/firebaseConnect.js Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@prescottprue prescottprue changed the base branch from next to v3.0.0-alpha.7 January 16, 2019 00:40
@prescottprue prescottprue changed the base branch from v3.0.0-alpha.7 to next January 16, 2019 00:40
@prescottprue
Copy link
Owner

prescottprue commented Jan 16, 2019

Great work on this @theashguy and @iamthefox , and thanks a ton for contribution. It is awesome to see the full feature, the tests, and sticking with the PR to solve merge conflicts.

@prescottprue prescottprue merged commit 8b3b7f9 into prescottprue:next Jan 16, 2019
@prescottprue prescottprue mentioned this pull request Jan 16, 2019
3 tasks
prescottprue added a commit that referenced this pull request Feb 5, 2019
* fix(firebaseConnect): rename internal props and throw for name collisions - #613 - @theashguy + @iamthefox
* fix(HOCs): missing props (firebase + dispatch) added - #606
* feat(typings): major upgrade to typescript definitions (including types from `@firebase`) - #627 - @rscotten
@prescottprue
Copy link
Owner

prescottprue commented Feb 9, 2019

This is causing some issues with the usage of the firebase prop downstream (see #582). I do like the idea of handling the case of this prop being passed and throwing, but I think renaming or disabling the firebase prop will have to be an option, at least for now.

Going to keep in the throwing for passed reserved prop names, but add back the firebase & dispatch props.

Really it is better to pass sections of the state from the reducer instead of the whole state anyway. That said, I do think there needs to be better naming conventions.

prescottprue added a commit that referenced this pull request Feb 9, 2019
fix(tests): add case to firebaseConnect to confirm firebase and dispatch props
@prescottprue prescottprue mentioned this pull request Feb 9, 2019
3 tasks
prescottprue added a commit that referenced this pull request Feb 9, 2019
* fix(firebaseConnect): add back firebase + dispatch props - #582, #613
* fix(typings): update more types including orderBy options
* fix(tests): add case to firebaseConnect to confirm firebase and dispatch props
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.

3 participants