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

fix: console dir should respect options #10638

Merged
merged 11 commits into from
Nov 10, 2020
Merged

fix: console dir should respect options #10638

merged 11 commits into from
Nov 10, 2020

Conversation

reckter
Copy link
Contributor

@reckter reckter commented Oct 15, 2020 •
edited by jeysal
Loading

Summary

This is a reopening of #10182, because it seemed stale.
Fixes #10176.
Closes #10182.

Test plan

(nothing changed except updating current master)

@reckter
Copy link
Contributor Author

reckter commented Oct 15, 2020

I think an argument could be made, if this truly is a bug-fix vs a breaking change. It does break console.dir({a: "test"}, {b: "test", depth: 200}), wich wil now no longer print out "b".
But I leave that up to a maintainer to decide :)

jeysal
jeysal reviewed Oct 15, 2020
View reviewed changes
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Thanks! I think it's fine as a fix, we're always trying to behave relatively similar to Node.

packages/jest-console/src/__tests__/CustomConsole.test.ts Show resolved Hide resolved
@jeysal
Copy link
Contributor

jeysal commented Oct 15, 2020

Great that you left the commit from #10182 intact, so @xamgore will still be an author in the merged commit 🙃

@jeysal jeysal mentioned this pull request Oct 15, 2020
Closed
@reckter
Copy link
Contributor Author

reckter commented Oct 15, 2020

@jeysal Got to leave that intact! I didn't code it, merely merge it again :) (well got to do a commit now for the test, but it's about the fix, not the code for me anywhay ^^)

@xamgore
Copy link
Contributor

xamgore commented Oct 15, 2020 •
edited
Loading

Thank you! 😌

SimenB proposed to use snapshots, what do you think about it?

@reckter
Copy link
Contributor Author

reckter commented Oct 15, 2020

@xamgore I do not now enough about the inner workings of console in different node.js versions to make an informed decision about it. For me it's fine either way. Currently we do not rely on specifics of the node.js implementation (I mean besides depth, which exposed the error) , which is probably a good thing.
But that's just my 2cents

jeysal
jeysal approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@jeysal jeysal 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 this is good to go as is 👍

@jeysal jeysal requested a review from SimenB October 15, 2020 16:55
@jeysal
Copy link
Contributor

jeysal commented Oct 19, 2020

@SimenB any objections? Seems likely now we'll land it for a major anyway (even though I'd consider it not breaking)

@jeysal
Copy link
Contributor

jeysal commented Oct 19, 2020

Adding this to the next major milestone, might as well land it in there

@jeysal jeysal added this to the Jest 27 milestone Oct 19, 2020
@reckter reckter changed the title Fix console dir Fix: console dir Oct 27, 2020
@reckter
Copy link
Contributor Author

reckter commented Oct 27, 2020

Uh I have no idea, why that one test failed in the latest merge failed. If you have any insides, i would love to hear them! :)

SimenB
SimenB approved these changes Oct 27, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
reckter and others added 2 commits November 6, 2020 19:01
@reckter @SimenB
Update CHANGELOG.md
098e20e 
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@reckter
Merge branch 'master' into fix-console-dir
eb04ed0
@reckter
Copy link
Contributor Author

reckter commented Nov 6, 2020

(Sorry for the late merge from master, busy few days, but here you go 👍 )

@reckter
Copy link
Contributor Author

reckter commented Nov 10, 2020

Fixed import errors, should be good to merge now :)

@SimenB SimenB changed the title Fix: console dir fix: console dir should respect options Nov 10, 2020
@SimenB SimenB merged commit 5deaa01 into jestjs:master Nov 10, 2020
@reckter reckter deleted the fix-console-dir branch November 10, 2020 11:03
jeysal added a commit to mmkal/jest that referenced this pull request Nov 10, 2020
@jeysal
Merge branch 'master' into uglier-inline-snapshots
be68e58 
* master:
  chore(docs): update TutorialReactNative.md (jestjs#10802)
  fix: console dir should respect options (jestjs#10638)
  docs: add missing "--roots" options (jestjs#10793)
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeysal jeysal jeysal approved these changes

@SimenB SimenB SimenB approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
Jest 27
Development

Successfully merging this pull request may close these issues.

console.dir doesn't respect depth option
5 participants
@reckter @jeysal @xamgore @SimenB @facebook-github-bot