[Fix] dive should pass along options from shallow
#771
Conversation
ljharb
changed the title
dive should pass along options from shallow
| @@ -3,6 +3,7 @@ import flatten from 'lodash/flatten'; | |||
| import unique from 'lodash/uniq'; | |||
| import compact from 'lodash/compact'; | |||
| import cheerio from 'cheerio'; | |||
| import merge from 'lodash/merge'; | |||
There was a problem hiding this comment.
we have object.assign available, please use that instead of adding a new dependency.
There was a problem hiding this comment.
I think we needs merge because options might be nested object.
// this.options === { context: {foo: 'hello'}};
// options === {context: {bar: 'enzyme!'}};
assign({}, this.options, options) // It will be {context: {bar: 'enzyme!'}}
There was a problem hiding this comment.
deep merging the options sounds like too much, i think eg merging the context as in your example should be done manually by the user. thoughts?
There was a problem hiding this comment.
Yes, I think we want to avoid deep merging.
There was a problem hiding this comment.
oh, okay. i got it. I'll fix this and then send PR 😄
thanks for your feedbacks 👍
| return this.single(name, (n) => { | ||
| if (isDOMComponentElement(n)) { | ||
| throw new TypeError(`ShallowWrapper::${name}() can not be called on DOM components`); | ||
| } | ||
| if (!isCustomComponentElement(n)) { | ||
| throw new TypeError(`ShallowWrapper::${name}() can only be called on components`); | ||
| } | ||
| return new ShallowWrapper(n, null, options); | ||
| return new ShallowWrapper(n, null, merge({}, originalOptions, options)); |
There was a problem hiding this comment.
I think this can use this.options inline - the arrow function will ensure this is correct.
There was a problem hiding this comment.
you're right. okay, I'll fix it.
|
@ljharb this might be a major, no? it can break existing tests for components that depend on context being available/not available. |
|
@nfcampos my thinking is that a component that expects context would never be used or tested without context - thus, what would be the use case for having a test that this change breaks? This also will pass through the lifecycle flag, but we're planning to enable that by default in the next major anyways. |
|
I fixed it. I tried to apply your feedbacks. Could you check it out? thanks! |
|
@ljharb only got to this now — ive been without internet at home for the better part of two weeks :( — thinking about this some more I could agree with this being a patch, arguably the previous behaviour was wrong, the expectation in react for a child to receive the same thing in context as its parent, what do you think? |
|
I think I agree that it should be a patch. |
|
@blackpost38 do you mind rebasing this on latest master? |
|
@nfcampos It's done. |
|
Thanks @blackpost38 ! |
I made it, could you check this source?
I only added a logic that merges options to
dive()refs: #770