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.

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

Remove console.warn of babel-plugin-import when the tests are running #5345

Closed
aaronplanell opened this issue Mar 16, 2017 · 12 comments
Closed
Labels
help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.

Comments

@aaronplanell
Copy link
Contributor

Environment(required)

  • antd version: 2.8 & 2.9
  • OS and its version: Ubuntu 16.04
  • Browser and its version: Chrome 57.0.2987.98 (64-bit)

What did you do? Please provide steps to re-produce your problem.

Run npm test in:

What do you expected?

No warnings.

What happen?

Both projects always show this message:

PASS  components/locale-provider/__tests__/index.test.js
  Console
    console.warn components/index.tsx:60
      You are using a whole package of antd,
      please use https://www.npmjs.com/package/babel-plugin-import to reduce app bundle size.

I think it will be important remove it when you're executing the tests.

I think, too, the solution is change this kind of code:

if (process.env.NODE_ENV !== 'production') {
    if (typeof console !== 'undefined' && console.warn && typeof window !== 'undefined') {
        console.warn(`You are using a whole package of antd,
please use https://www.npmjs.com/package/babel-plugin-import to reduce app bundle size.`);
    }
}

Providing another conditional for remove warnings if you are lauching the tests. What do you think about it?

Re-producible online demo

Just download the antd project and run npm test.

@benjycui
Copy link
Contributor

Yep, we should check environment before printing any warning.

Could you just PR to fix this? Thanks.

@benjycui benjycui added the help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request. label Mar 17, 2017
@benjycui
Copy link
Contributor

More info:

➜  ant-design git:(master) grep -r 'warning(' ./components | grep tsx
./components/_util/warning.tsx:    warning(false, message);
./components/badge/index.tsx:    warning(
./components/breadcrumb/Breadcrumb.tsx:    warning(
./components/breadcrumb/Breadcrumb.tsx:        warning(
./components/date-picker/Calendar.tsx:    warning(false, 'DatePicker.Calendar is deprecated, use Calendar instead.');
./components/date-picker/createPicker.tsx:      warning(!('onOK' in props), 'It should be `DatePicker[onOk]` or `MonthPicker[onOk]`, instead of `onOK`!');
./components/date-picker/RangePicker.tsx:    warning(!('onOK' in props), 'It should be `RangePicker[onOk]`, instead of `onOK`!');
./components/date-picker/wrapPicker.tsx:        warning(
./components/form/Form.tsx:        warning(
./components/form/Form.tsx:    warning(!props.form, 'It is unnecessary to pass `form` to `Form` after antd@1.7.0.');
./components/form/Form.tsx:    warning(
./components/form/FormItem.tsx:    warning(
./components/menu/index.tsx:    warning(
./components/message/index.tsx:  // Departed usage, please use warning()
./components/message/index.tsx:  warning(content: ConfigContent, duration?: ConfigDuration, onClose?: ConfigOnClose) {
./components/popover/index.tsx:    warning(
./components/table/Table.tsx:    warning(
./components/table/Table.tsx:    warning(recordKey !== undefined,
./components/tabs/index.tsx:    warning(
➜  ant-design git:(master) grep -r 'warn(' ./components | grep tsx
./components/index.tsx:    console.warn(`You are using a whole package of antd,
./components/message/index.tsx:  warn(content: ConfigContent, duration?: ConfigDuration, onClose?: ConfigOnClose) {

@aaronplanell
Copy link
Contributor Author

Hello,

In fact Antd currently check the enviroment:

if (process.env.NODE_ENV !== 'production') {
 etc...
}

Maybe something like:

if (process.env.NODE_ENV !== 'production' || process.env.NODE_ENV !== 'test') {
 etc
}

And modify the package.json:

    "test": "NODE_ENV=test jest",

Instead of:

    "test": "jest",

And, finally, modify the last two lines the file scripts\test-all.sh this way:

NODE_ENV=test jest npm test -- --coverage -w $MAX_WORKERS && \
NODE_ENV=test jest npm test -- --config .jest.node.json -w $MAX_WORKERS

...will work. What do you think about?

@afc163
Copy link
Member

afc163 commented Mar 17, 2017

Actually, I told @benjycui same solution this afternoon.

@trevorlitsey
Copy link

Hello,

I'm getting this same warning in my tests and can't seem to get rid of it even while setting NODE_ENV=test.

console.warn node_modules/antd/lib/index.js:499 You are using a whole package of antd, please use https://www.npmjs.com/package/babel-plugin-import to reduce app bundle size.

I'm using babel-plugin-import in my .babelrc and don't see these warnings in the dom, but have to leaf through many in my tests...

Any help is appreciated.

Thanks!

@larkintuckerllc
Copy link

Just updated to the latest release...

"antd": "^3.4.0",

and still seeing warnings with Jest with NODE_ENV=test

can set NODE_ENV=production and suppress the errors.

The weird thing is that it looks like the PR (merged into master on Mar 20)

#5397

appears to fix the current issue.

BUT... The current master does not show the changes (would guess that some later commit removed the fix???)

@larkintuckerllc
Copy link

Having slept on this overnight, had a change of heart. I am thinking it is best practice to be testing your code against the version that will be going into production. So setting NODE_ENV=production before running your test is likely a best practice. If I need to mock things in tests, I should be using Jest's features to do this instead of having the code behave differently under tests than production.

@andriijas
Copy link
Contributor

it was undone by @afc163 in #9814

Im kind of leaning toward @larkintuckerllc that test env should have similar config as prod and dev but even with react-app-rewire this is messy to fix with create-react-app and at the end of the day bundle size doesnt actually matter in test environment.

@punmechanic
Copy link

punmechanic commented Apr 24, 2018

Not only that, but this assumes the user is using Babel. I use Typescript with Webpack. I know that, because of Webpack, dead code is going to be eliminated. This warning is useless in this scenario and there's no way to turn it off.

I don't need this warning cluttering up my terminal. You shouldn't expect the user to change NODE_ENV to production to disable this. In tests, NODE_ENV should be test. You also really shouldn't encourage the user to know about the directory structure of your library. You have exposed a context module, as is the usual pattern; let users use it without being told off.

http://www.linfo.org/rule_of_silence.html

@vanshady
Copy link
Contributor

vanshady commented May 3, 2018

I second @danpantry 's opinion in that for our team, it's a design choice to not use babel-plugin-import (we are using next.js and sass-loader is configured very differently and doesn't quite support babel-plugin-import ). It's just annoying to keep seeing this message in testing environment.

@yesmeck
Copy link
Member

yesmeck commented May 4, 2018

Please file a new issue.

@andriijas
Copy link
Contributor

#10363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.
Projects
None yet
Development

No branches or pull requests

9 participants