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.

Sign up for GitHub

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

Jump to bottom

feat: Allow custom serializers through setSerializers #91

Merged
merged 1 commit into from
Nov 5, 2019
Merged

feat: Allow custom serializers through setSerializers #91

merged 1 commit into from
Nov 5, 2019

Conversation

alistairjcbrown
Copy link
Contributor

@alistairjcbrown alistairjcbrown commented Sep 15, 2019 •
edited
Loading

Provides new setSerializers function to set the serializer objects to
be used before diffing. React component serializer implementation using
react-test-renderer has been extracted out.

Fixes #18
Fixes #30
Fixes #31

@alistairjcbrown
Copy link
Contributor Author

@thymikee I haven't added any doc updates as part of this PR, but give me a shout if the direction look ok and I'll get them added to the branch

thymikee
thymikee reviewed Sep 23, 2019
View reviewed changes
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like the API, let's proceed with this. Thanks for the contribution :)

@alistairjcbrown alistairjcbrown force-pushed the feature/allow-custom-serializers branch from 01019d8 to bc42dd3 Compare September 23, 2019 16:00
@alistairjcbrown alistairjcbrown changed the title feat: Allow custom serializers through addSerializers feat: Allow custom serializers through setSerializers Sep 23, 2019
@alistairjcbrown
Copy link
Contributor Author

alistairjcbrown commented Sep 23, 2019 •
edited
Loading

@thymikee Thanks - updated the branch 👍

  • Added docs to readme
  • Tests updated to use nested component and now diff for fully rendered component, shallow rendered using react test renderer, shallow rendered using enzyme, POJO. Hopefully that shows off multiple serializers living together.
  • Looking at the issues I spotted Consider removing React rendering behaviour altogether #31, which notes wanting to be able to control how react components are rendered (amongst other things). As such I've updated this branch to instead setSerializers, rather than addSerializers - the react serializer is now not included by default when calling setSerializers. This serializer is exposed via snapshotDiff.reactSerializer and can be added to the list of serializers passed into snapshotDiff.setSerializers by users.

@alistairjcbrown alistairjcbrown force-pushed the feature/allow-custom-serializers branch from bc42dd3 to a15eb2d Compare September 23, 2019 16:13
@thymikee
Copy link
Member

Sorry for all the delay, I'll get to it next week. I'm pretty swamped with work right now 😅.

@alistairjcbrown
Copy link
Contributor Author

@thymikee No worries at all, give me a shout if there's anything else that needs updated 👍

@alistairjcbrown
Copy link
Contributor Author

@thymikee No pressure on this one - just making sure it hasn't fallen off the todo list

thymikee
thymikee requested changes Oct 27, 2019
View reviewed changes
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's apply the inline feedback and take it over the finishing line :) Thanks for being so patient with my review 🙈

README.md Outdated
const myCustomSerializer = require('./my-custom-serializer');

snapshotDiff.setSerializers([
snapshotDiff.reactSerializer // use default react serializer - add this if you want to serialise react components!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's export defaultSerializers object so that we can spread it here

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@thymikee
Copy link
Member

thymikee commented Nov 4, 2019

@alistairjcbrown do you need any help to get this over the finishing line?

@alistairjcbrown
feat: Allow custom serializers through setSerializers
6afc4a0 
Provides new setSerializers function to set the serializer objects to
be used before diffing. React component serializer implementation using
`react-test-renderer` has been extracted out.

Fixes jest-community#18
Fixes jest-community#30
Fixes jest-community#31
@alistairjcbrown alistairjcbrown force-pushed the feature/allow-custom-serializers branch from a15eb2d to 6afc4a0 Compare November 4, 2019 23:46
@alistairjcbrown
Copy link
Contributor Author

@thymikee Apologies, I hadn't caught up on my notifications. The branch has been updated - hopefully I've covered everything in the comments, but let me know if there's any more changes to do 👍

@thymikee thymikee changed the title feat: Allow custom serializers through setSerializers feat: Allow custom serializers through setSerializers Nov 5, 2019
@thymikee thymikee merged commit ae8e62b into jest-community:master Nov 5, 2019
@thymikee
Copy link
Member

thymikee commented Nov 5, 2019

Thank you very much for this contribution. It's now released under v0.6.0 :)

@SimenB
Copy link
Member

SimenB commented Nov 5, 2019

Missing update to https://github.com/jest-community/snapshot-diff/blob/master/index.d.ts 😱

@alistairjcbrown alistairjcbrown deleted the feature/allow-custom-serializers branch November 5, 2019 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thymikee thymikee thymikee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@alistairjcbrown @thymikee @SimenB