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

Replace warning #1192

Merged
merged 4 commits into from Dec 18, 2018

Conversation

2 participants
@jaredpalmer
Copy link
Owner

jaredpalmer commented Dec 18, 2018

Warning is not treeshakable :shrug:

jaredpalmer added some commits Dec 18, 2018

@jaredpalmer

This comment has been minimized.

Copy link
Owner

jaredpalmer commented Dec 18, 2018

Errrrrr. Actually, I'm not sure this will work. the warning module provides the callstack, which is kind of nice.

Also tiny-warning, uses console.warn while warning actually uses console.error under the hood

@jaredpalmer jaredpalmer requested a review from TrySound Dec 18, 2018

@jaredpalmer jaredpalmer changed the title Replace warning with tiny-warning Replace warning Dec 18, 2018

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Dec 18, 2018

Why error instead of warning? Error always frighten me when I see it in console.

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Dec 18, 2018

I can add call stack
alexreardon/tiny-warning#2

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Dec 18, 2018

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Dec 18, 2018

Could you point me how throwing error helps with debugging? I see stack trace in both cases
https://codesandbox.io/s/l34rvm5r8z

@jaredpalmer

This comment has been minimized.

Copy link
Owner

jaredpalmer commented Dec 18, 2018

Warning uses console.error. there is a full discussion about it in React repo somewhere. Not sure why we would change this behavior now.

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Dec 18, 2018

I found it. I understand why try catch there. But console.error reason seems like a legacy. I see stack trace with warning too. I just think that semantically error is too aggressive.

facebook/react#4216

@jaredpalmer

This comment has been minimized.

Copy link
Owner

jaredpalmer commented Dec 18, 2018

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Dec 18, 2018

Should we? The decision was made because of features lack in old browsers. Now it's probably a time to follow semantic. We may even rise an issue in react so they could migrate in v17.

@jaredpalmer

This comment has been minimized.

Copy link
Owner

jaredpalmer commented Dec 18, 2018

Fair enough. We just need to update test harness.

@jaredpalmer

This comment has been minimized.

Copy link
Owner

jaredpalmer commented Dec 18, 2018

@jaredpalmer

This comment has been minimized.

Copy link
Owner

jaredpalmer commented Dec 18, 2018

This should work now

@jaredpalmer jaredpalmer merged commit 13954fd into master Dec 18, 2018

5 checks passed

WIP ready for review
Details
ci/circleci: deploy-docs Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
security/snyk - package.json (jaredpalmer) No new issues
Details
security/snyk - website/package.json (jaredpalmer) No new issues
Details

@jaredpalmer jaredpalmer deleted the feature/tiny-warning branch Dec 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment