Separate fixture props #312
Conversation
There was a problem hiding this comment.
This is great, thanks so much for picking it up!
- Please upgrade with latest master, there were a few PRs merged since you started this branch
- Leaving out the codemod is fine, but we can't deliver this without the proxy because it will break user's integration without an easy upgrade path (those who already depend on the 2.0.0 beta). The good news is you've already implemented the proxy, we just need to wrap it up nicely. I've added some comments inline. It should also be created as an independent package (see context or Redux proxies). I can help with structure/docs.
- We should turn the temp example into a real example. Either that or – in order to not duplicate the Counter code – we just use the proxy in the
local-stateexample and have both new style and old style fixtures in the same example - You also need to update StateProxy
Lastly, there's an unresolved issue with this. Let's find an elegant solution together. Cosmos has this concept of splitting serializable and unserializable fixture parts. We do this to be able to serialize part of the fixture into JSON and make it editable in the fixture editor. The unserializable parts are hidden, but passed to the component, while the serializable ones are visible and editable. This separation is shallow, meaning that if a nested fixture attribute isn't serializable, it's completely hidden. Long story short, now that props are nested under fixture.props, if you have any prop callback no prop will be shown in the editor.
All props are serializable:
Just one props is unserializable:
This might be a lot to chew, so let me know if you have any questions. You've just opened the Cosmos Pandora's box! 🤡
| } | ||
| // TODO: bring notProps from cosmos config | ||
| const notProps = ['children', 'state']; | ||
| const fixedFixture = reduce(fixture, (final, value, key) => { |
There was a problem hiding this comment.
You can use lodash.omit to replace the reduce logic into a single line
| return fixture; | ||
| } | ||
| // TODO: bring notProps from cosmos config | ||
| const notProps = ['children', 'state']; |
There was a problem hiding this comment.
Add reduxState and we might postpone making this an option indefinitely (unless anybody requests it).
| // the fixture doesn't need fixing if it has props | ||
| return fixture; | ||
| } | ||
| // TODO: bring notProps from cosmos config |
There was a problem hiding this comment.
Even if this list were configurable (which I don't recommend for now), it shouldn't be an option inside cosmos.config.js, but rather a Proxy option (see example of Proxy options in Redux Proxy).
| return fixedFixture; | ||
| }; | ||
|
|
||
| class FixFixturePropsProxy extends React.Component { |
There was a problem hiding this comment.
How about NormalizePropsProxy?
|
Well explained... I'll make these changes before the end of the weekend. Wondering how many levels deep I need to check to find unserializable props? I'll start with props root for starters. |
|
One level deep (root props) is as good as before. Ideally the splitting would be recursive and would be any level deep. Wondering if there's an easy algorithm for this. The key here is how we merge the serializable/unserializable parts back, which until now was trivial. Finally, as further context, my wish is to someday add a notice at the bottom of the fixture editor like this:
|
use splitResult to get around linter no-extend-native
|
Still todo:
|
There was a problem hiding this comment.
StateProxy passes fixture untouched to PropsProxy now
This is good. Why does it worry you?
I use _.pick + _.omit in the proxy because I thought we don't want to leave behind what we transfer to fixture.props
Also good. Provided some ideas for improvement inline.
Make NormalizeStateProps a working package (not sure what is needed here as far as lerna and tooling config)
Should already be fine. Run npm run build in the root dir to make sure the lib is generated.
Finish backwards compatibility example (my vote is to move it in a folder separate from /examples to avoid confusion with old API)
👍
Thanks again for this massive contribution!
| @@ -83,7 +83,7 @@ describe('fixture without state and component without initial state', () => { | |||
|
|
|||
| commonTests(); | |||
|
|
|||
| test('omits state from fixture sent to next proxy', () => { | |||
| test('does not omit state from fixture sent to next proxy', () => { | |||
There was a problem hiding this comment.
I think this test case is useless and can be removed
| unserializable: { } | ||
| }; | ||
|
|
||
| const { serializable, unserializable } = splitResult; |
There was a problem hiding this comment.
This looks convoluted. Why not go back to just
const serializable = {};
const unserializable = {};
// ...
return { serializable, unserializable };|
|
||
| Object.keys(obj).forEach(key => { | ||
| if (isSerializable(obj[key])) { | ||
| serializable[key] = obj[key]; | ||
| } else if (key === 'props' && isPlainObject(obj[key])) { | ||
| Object.keys(obj.props).forEach(propKey => { | ||
| let propHome = 'serializable'; |
There was a problem hiding this comment.
Moreover, you could do something like
const propVal = obj.props[propKey];
const propHome = isSerializable(propVal) ? serializable : unserializable;
if (!propHome.props) {
propHome.props = {};
}
propHome.props[propKey] = propVal;There was a problem hiding this comment.
But it would be wise to add the new tests first and then beautify the implementation.
| return fixture; | ||
| } | ||
|
|
||
| const notProps = ['children', 'state', 'context', 'reduxState']; |
There was a problem hiding this comment.
You can put this outside of getFixedFixture since they never change.
| const fixedFixture = Object.assign(pick(fixture, notProps), { | ||
| props: omit(fixture, notProps) | ||
| }); | ||
| return fixedFixture; |
There was a problem hiding this comment.
How about
return {
...pick(fixture, notProps),
props: omit(fixture, notProps)
};| { | ||
| "name": "react-cosmos-normalize-props-proxy", | ||
| "version": "2.0.0-beta.6", | ||
| "description": "Seperate props in fixtures", |
There was a problem hiding this comment.
"Proxy for backwards compatibility with old react-cosmos fixture format"
| class NormalizePropsProxy extends React.Component { | ||
| render() { | ||
| const { nextProxy, fixture } = this.props; | ||
|
|
| export default { | ||
| componentPaths: ['components'], | ||
| // TODO: use package for normalize proxy | ||
| proxies: ['../../packages/react-cosmos-normalize-props-proxy/src'] |
There was a problem hiding this comment.
No need for a TODO since examples are explicitly working with relative packages, but you should rely on the compiled lib by just pointing to '../../packages/react-cosmos-normalize-props-proxy'
|
Awesome... looks like this is pretty close to done. Review makes sense to me, so I'll make the changes as suggested. Should be able to get it done during the week, if not definitely over next weekend. |
|
@skidding Implemented feedback and added tests. Not sure where you want the NormalizePropsProxy example to live. I've seen projects split up the examples folder into e.g. |
| expect(childProps.fixture.context).toEqual({}); | ||
| expect(childProps.fixture.children).toEqual([]); | ||
| expect(childProps.fixture.state).toEqual({}); | ||
| expect(childProps.fixture.reduxState).toEqual({}); |
There was a problem hiding this comment.
These checks can become false positives in time. How about creating some constants like const mockContext = {}, use them when constructing the fixture in renderProxy calls and then do strict checks like expect(childProps.fixture.context).toBe(mockContext);
|
Discovered a problem with how fixture parts are merged. The problem is when you have one or more unserializable props and one or more serializable props, the unserializable props are overridden by the serializable ones. We need to merge the parts recursively. _.merge should do the trick. Additionally, while trying to integrate in own projects I realized I had more magic fixture fields that aren't props (designed to be picked up by custom proxies), which are being normalized into props. It would help to make Thanks! 🙌 |
Yeah, sure. No telling what kind of magical custom proxies people are running. To pass the options, the correct approach is to wrap the proxy, e.g. as seen here, correct? Will fix the bug too. Not totally clear on why the fixture is split and then merged again (why not just use the original fixture?), so will read more into that. Should be able to get this done tomorrow. |
Yes... but in that example we're composing proxies. I think the Redux proxy is also good inspiration. The idea is to put the whole proxy component inside a create function, in order to have access to the options passed to that create function.
Because of the fixture editor. Users can edit the serializable part of the fixture, which is merged with the unserializable part (you could call that part immutable) to form the new fixture.
Let me know if this makes sense. Thanks! |
Makes sense!
I think my question is geared towards composing proxies. How would the user pass options to the new |
|
Oh, you're right about the proxy wrapping, that's how you pass options. It's also seen in the two proxy examples from the README: https://github.com/react-cosmos/react-cosmos#react-cosmos-redux-proxy In the future we could allow proxy options inside cosmos.config (similar to webpack loader options), but I don't think Cosmos is popular enough yet for that sort of customization :P |
Yeah, I'm not sure the overall idea of developing components in isolation has hit mainstream yet. People seem more keen on the UI explorer/documentation aspect. Sometimes the killer feature of a project is misidentified early on, and it isn't until later that people realize that the better benefit is something else. Personally, I'm only doing cosmos-first development from now on, where cosmos playground is the entry point and you delay writing nav / router type components that can distract from the app's idea. |
Yes, I couldn't agree more! ❤️
Same here! 🥂 |
|
@skidding Let me know if you think of anything else. A new test to cover the serializable overwriting unserializable crossed my mind, but wasn't immediately obvious how to tackle that. I imagine such a test would live with the fixture editor tests. |
|
I think this is great and ready for prime time. 🥇 @ovidiubute @flaviusone @RadValentin you might want to take a look at this! It is breaking, so we need to make sure your react-cosmos version(s) are locked at |
|
We're good on our end, the other guys are still on Fire away! 💣 |
|
Great contribution @jlc467, thanks again making this happen! Oh, and I sent you an invitation to become a collaborator 🙌 |
|
@skidding Sweet! And thanks for helping me with the contributions 🙏. I know README updates are still missing for both this and cosmos-export, so if that hasn't been picked up in the next couple weeks, I will try to contribute there. |
|
@jlc467 Welcome on board! ⚔️ Here are two examples where I've already upgraded: Regarding docs, sure! I hope the latest structural README changes make it easier to contribute. You're also invited to join the Slack channel. |
Issue: #217
With this PR, fixture props are expected to be attributes of
fixture.propsinstead of the the root-levelfixture.Also in this PR: local-state example shows how to use props in a fixture.
To test backwards compatibility, I copied the local-state example and put props at the root-level, then added a
FixFixturePropsProxyto cosmos config to confirm the props come through. Before merging, I imagine this example will be deleted and the proxy extracted into some kind of markdown documentation in the release notes.Haven't looked into codemods yet, but figure that can come later.