Add hook and forwardRef support #1027
Conversation
| } | ||
| } | ||
|
|
||
| export function renderFcIntoDocument(element) { |
There was a problem hiding this comment.
@Radium (at least on class components) now exports with a HoC to inject context which is written as a function component, and TestUtils.renderIntoDocument requires a class component
| const Enhanced = Enhancer(Composed); | ||
|
|
||
| const instance = new Enhanced(); | ||
| const ref = React.createRef(); | ||
| renderFcIntoDocument(<Enhanced ref={ref} />); |
There was a problem hiding this comment.
Class components are now wrapped in a HoC for injecting the contexts, so using ref to get access to the instance
| const Enhanced = Enhancer(Composed); | ||
|
|
||
| const instance = new Enhanced(); |
There was a problem hiding this comment.
can't do tests like this as class components are now wrapped in a HoC to inject context
| const {radiumConfig} = props; | ||
|
|
||
| const configContext = useContext(RadiumConfigContext); | ||
| const styleKeeper = useRef<StyleKeeper>( |
There was a problem hiding this comment.
this won't change if radiumConfig prop changes -- but that is true to the original implementation
src/enhancer.js
Outdated
| getRadiumStyleState(this) | ||
| if (this.props.radiumConfig) { | ||
| return ( | ||
| <RadiumConfigContext.Provider value={this.props.radiumConfig}> |
There was a problem hiding this comment.
previous version would override childContext with the config prop if provided
nathanmarks
force-pushed
the
add-hook-support
branch
from
d360b29
to
249c016
Compare
| // need to attempt to assign to this.state in case | ||
| // super component is setting state on construction, | ||
| // otherwise class properties reinitialize to undefined | ||
| state: Object = this.state || {}; |
There was a problem hiding this comment.
This is due to a change in the class properties behaviour in babel 7.
nathanmarks
force-pushed
the
add-hook-support
branch
from
76e42da
to
8285a31
Compare
|
@nathanmarks major appreciation for this PR - We've been moving towards placing Radium into a more stable maintenance mode and avoiding larger refactors, but this type of work warrants a semver minor release considering the current version of Radium straight up does not support hooks. We'll work to get this merged and released in |
|
Hello @nathanmarks! I wanted to see if there was any assistance I could provide to aide with this pull request :) I have a few spare cycles this week and can help with review or getting the branch ready for merge. |
|
@kylecesmat apart from one of the things I highlighted in a comment that needs covering still in the function component enhancer and some additional DRY-ing up between the class and function enhancers, would be great if you could give this a thorough review. Do you guys have any codebases still running Radium that this could be tested on to validate it? (I noticed you had moved to Would be great to make sure we haven't accidentally introduced any issues that aren't covered in the test suite (such as FOUC problems with media queries: #626) Thanks so much!! |
|
@nathanmarks Thank you so much for putting this together! I tried out your changes in our app to see how they work with our pretty complex implementation and to make it possible to use hooks in our radium-connected components, and so far so good! To make it easy to test and easy to deploy a version of our app that uses your work, I made a repo with all the built radium files from your branch, which can be directly used as the dependency string with npm/yarn. In the - "radium": "0.25.2",
+ "radium": "brandcast/radium-built#c7bd574", |
|
Thanks @acusti but that shouldn’t actually be necessary. Radium uses the ‘publishr’ ( https://github.com/FormidableLabs/publishr ) tool to to add build deps and a postinstall to actually do a build when installing from git branch, and does none of that when installing from npm/yarn (which has the built, not-in-git dist files). Did you find something wrong with a direct git branch dependency? |
That’s clever! I didn’t know, so I just manually ran all the build commands I found in the package.json (build-lib, build-dist, build-es), then copied those built files over into a bare repo ( "radium": "nathanmarks/radium#add-hook-support", |
|
@ryan-roemer I tried with the direct git branch dependency and the |
|
@acusti -- Thanks. I'll take a look. |
|
@acusti -- good catch. This PR breaks the @nathanmarks -- I'm going to create a patch for you that will hopefully work for our |
|
@acusti @nathanmarks -- this should fix the diff --git a/package.json b/package.json
index 72b92e5..7e04935 100644
--- a/package.json
+++ b/package.json
@@ -61,6 +61,7 @@
"@babel/plugin-syntax-dynamic-import": "^7.0.0",
"@babel/plugin-transform-destructuring": "^7.0.0",
"@babel/preset-env": "^7.0.0",
+ "@babel/preset-flow": "^7.0.0",
"@babel/preset-react": "^7.0.0",
"babel-loader": "^8.0.0",
"builder": "^3.2.3",
@@ -76,9 +77,7 @@
"react": "^16.8.0"
},
"devDependencies": {
- "@babel/core": "^7.0.0",
"@babel/node": "^7.0.0",
- "@babel/preset-flow": "^7.0.0",
"babel-eslint": "^10.0.1",
"babel-plugin-istanbul": "^5.1.0",
"caniuse-api": "^2.0.0", |
| }, | ||
| "devDependencies": { | ||
| "babel-eslint": "^7.1.1", | ||
| "@babel/core": "^7.0.0", | ||
| "@babel/node": "^7.0.0", |
|
@kylecesmat sorry, had a really busy couple of weeks! Are you guys still wanting to see this get in as the final big update to Radium? |
|
@nathanmarks absolutely no worries, thank you for taking time out of your life to contribute to this project :) We'd love to include this feature in Radium. I think we're close, let me know if you're able to address the feedback above and we'll get it merged + released. |
|
@nathanmarks @kylecesmat we (at the company i work for) would also love to see this feature in radium! can we help get it over the finish line? not sure how hard it would be for someone else to get up to speed on everything in this PR, but we might be able to dedicate an engineer to take it over and ship it if that could help. |
|
Sorry for dragging my feet on this! Let me see if I can find some time tomorrow or Thursday to address feedback and wrap up my changes. If I don't, I'm happy to work with someone to finish this up! |
|
Started to finish up |
nathanmarks
changed the title
|
@ryan-roemer following up on the publishr workflow, i just retried the |
nathanmarks
force-pushed
the
add-hook-support
branch
from
1a93b4b
to
ae346b0
Compare
nathanmarks
force-pushed
the
add-hook-support
branch
from
ae346b0
to
00f12a8
Compare
|
btw, the coverage report on master is a little messed up 🤔 there's an issue with the line #s after a certain point in most files due to how the webpack rules were configured. I've fixed this. This has resulted in a change in coverage for some files that I haven't modified. For eg: My Build |
|
@acusti how's the latest working for you? I think everything is done 🤔 |
|
@nathanmarks it’s working great! we put it through QA, found no issues, and are currently using it in production 🚀 |
|
@kylecesmat based on:
sounds like this is ready for a re-review! |
|
@acusti @nathanmarks standby for some code review, will try to get this merged & released today! |
|
Pulled this down and ran the suites/verified publishing. Lets merge this and get it released as |
Addresses:
#1016
#1010
This PR makes some big changes, and I understand if dropping support for React < 16.8 on master is not do-able at this time. However, the inability to use hooks and forwardRef with Radium means we are missing out on some of the great features being introduced to React.
Hopefully the changes in here can at least serve as a point for discussion (if everything works as-intended).
enhancer.js)I also had to upgrade babel, eslint + plugins, and flow to be able to use the latest version of React features + their associated flow types.
I had to comment out node 6 for appveyor because the eslint react hooks plugin is asking for node 7+