-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
merge existing withConfig arguments to allow SSR when using shouldForwardProp #323
Conversation
07da296
to
2a09ad2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "optional chaining" improves the readability of the code. How about applying this?
@1Seok2 I did not see optional chaining used anywhere else in the code so i instead matched convention with what was there. I assume thats intentional to support versions of node that do not support chaining. |
@probablyup can I get a review on this? |
@ithinkdancan sorry for the delay in reviewing, can you also add a test case for |
@probablyup added. works as expected if withConfig is after attrs, seems to have issues if they are swapped and you get 2 withConfigs. could maybe use some ideas on how to remedy that |
Hmm that won't work, we need it to work in both places or at least before, per the docs:
|
@probablyup good call out, I have updated the code to be a little smarter about merging the withConfig arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TY
@ithinkdancan could you merge from |
@probablyup rebased onto main and pushed it up. |
Awesome, thank you. |
@ithinkdancan waiting on a couple other small PRs and then will cut a release with this in it, thanks for your contribution! |
Resolves issue #322 where if you have an existing withConfig on your component in order to leverage shouldForwardProp babel was not adding a displayName or componentId
this takes the existing expression adds the additional args and replaces the existing withConfig