-
-
Notifications
You must be signed in to change notification settings - Fork 384
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: Add ignoreOrder flag #422
Conversation
|
Can someone from mainteiners merge this please. |
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.
Please add test with ignoreOrder: false
@evilebottnawi technically all the other tests are testing ignoreOrder: false, but i've added an explicit test. |
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.
We need check stats.warning
is empty or not in tests otherwise tests doesn't make sense
@evilebottnawi added |
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.
Good job, three notes
test/IgnoreOrder.test.js
Outdated
cache: false, | ||
}); | ||
compiler.run((err1, stats) => { | ||
expect(stats.hasWarnings()).toBeTruthy(); |
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.
toBe(true)
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.
just wondering why toBeTruthy(); is "better". Seems like the exact same shit to me...
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.
test/IgnoreOrder.test.js
Outdated
cache: false, | ||
}); | ||
compiler.run((err1, stats) => { | ||
expect(stats.hasWarnings()).toBeFalsy(); |
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.
toBe(false)
test/IgnoreOrder.test.js
Outdated
@@ -0,0 +1,44 @@ | |||
import path from 'path'; |
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.
rename file ignoreOrder-option.test.js
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.
👍
Do we need more approvals? |
Thanks @evilebottnawi! 🎉 |
Yeah, thanks a lot guys! |
Today |
This PR contains a:
Motivation / Use-Case
Picking up the torch where #366 left off and making the changes suggested by @evilebottnawi
Also addresses #362
Borrowing @hedgepigdaniel 's description:
As has been pointed out in #250:
Breaking Changes
None
Additional Info
The added tests is to ensure that enabling the flag doesn't make the plugin implode, but it doesn't really programmatically verify the warnings aren't printed.
fixes #362