-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
feat(pretty-format): allow to opt out from sorting object keys with compareKeys: null
#12443
Conversation
…compareKeys: null`
7da9111
to
434afe7
Compare
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
I think that the PR is still sound. |
Would be good to tweak docs here ( I just think it might be tricky to explain the behaviour in a simple way. Perhaps |
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
packages/pretty-format/README.md
Outdated
@@ -69,7 +69,7 @@ console.log(prettyFormat(onClick, options)); | |||
| key | type | default | description | | |||
| :-------------------- | :-------- | :--------- | :------------------------------------------------------ | | |||
| `callToJSON` | `boolean` | `true` | call `toJSON` method (if it exists) on objects | | |||
| `compareKeys` | `function`| `undefined`| compare function used when sorting object keys | | |||
| `compareKeys` | `function|null`| `undefined`| compare function used when sorting object keys, `null` can be used to skip over sorting | |
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.
table should be realigned
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.
done 👍
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.
same for the other one 😀
there's also a type error here, but I guess you might know that already? (I need to read through the issues here, I've forgotten all context)
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've fixed both - let me know if the solution for the types of problem satisfies your needs. The problem was that when DEFAULT_OPTIONS
were typed as Options
they included a function type on the snapshotFormat
property. This was OK for the pretty-format
lib itself but later on, those DEFAULT_OPTIONS
were used in the jest-config
where the function type on that property is not valid. By changing the type to a subtype of Options
we don't lose any type-safety and we fix the problem because now DEFAULT_OPTIONS
don't include that function type there and that runtime object just doesn't include snapshotFormat
property.
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.
this seems fine to me - let's do it! 😀
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This supersedes #12387 . However, it doesn't solve the full problem because the validation logic for the config is still based on
DEFAULT_OPTIONS
from thepretty-format
and thus this validcompareKeys: null
won't pass the validation at the config level.So this PR only lays out part of the solution for the whole problem.
I've decided to use
null
for this to avoid special-casing some strings if you ever come around to implementingcompareKeys: string
that would allow for loading modules containing thecompareKeys
export, like suggested in #12387 (comment) . I'm not attached to this solution so if you prefer to use special strings as suggested here: #12387 (comment) , then I'm open to adjusting this PR accordingly.cc @SimenB