Fix warning without stack for ie9 #13620
Conversation
Details of bundled changes.Comparing: f6fb03e...f785a3d schedule
Generated by 🚫 dangerJS |
| const originalConsoleError = console.error; | ||
|
|
||
| if (typeof console.error.apply === 'undefined') { | ||
| console.error = consoleError; |
There was a problem hiding this comment.
There are more moving parts here than I'd like.
I'd like to use the same solution for all cases. Can we do that somehow?
The easiest one might just be to switch on args length and explicitly pass them all down.
There was a problem hiding this comment.
That was my first intention :)
But then I thought that maybe it’s not desired to switch to fixed amount of args.
If it’s ok for you - I can update.
There was a problem hiding this comment.
Fixed amount of args is okay. We have that in invariant.
|
All the parameters are passed explicitly now, but result looks a bit frustrating for me.. So please have a look, thanks! |
| @@ -15,8 +15,6 @@ | |||
| let warningWithoutStack = () => {}; | |||
|
|
|||
| if (__DEV__) { | |||
| const consoleError = Function.prototype.bind.call(console.error, console); | |||
|
|
|||
| warningWithoutStack = function(condition, format, ...args) { | |||
There was a problem hiding this comment.
Let’s assert the number of arguments isn’t higher than supported here?
There was a problem hiding this comment.
As far as I can see the maximum of 8 arguments is used at the moment for this warning. Do you want to add extra check? But how many arguments do we need to check? That's exactly the reason I didn't use this approach initially..
btw, the tests marked as failed, but as I can see on circleci - tests itself passed, but later this happened. Is it related to the change? Or verification job could be just restarted?
Request failed [401]: https://api.github.com/repos/facebook/react/pulls/13620/requested_reviewers
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Error: TypeError: Cannot read property 'repo' of undefined
at GitHub.APIMetadataForPR (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:274:27)
at GitHub. (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:164:39)
at step (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:40:23)
at Object.next (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:21:53)
at fulfilled (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:12:58)
at
at process._tickCallback (internal/process/next_tick.js:189:7)
Danger failed
There was a problem hiding this comment.
If 8 is current maximum the let’s check we have at most 8.
There was a problem hiding this comment.
Try rebasing to solve the CI issue
|
This sounds a bit crazy, but I wonder if |
|
@Kovensky not really as far as I can see.. but interesting idea. |
link-alex
force-pushed
the
fix-warning-without-stack-ie9
branch
from
92ec2a3
to
c536a4e
Compare
Where console methods like log, error etc. don't have 'apply' method. Because of the lot of tests already expect that exactly console['method'] will be called - had to reapply references for console.error method facebook#13610
which is not supported for console methods in ie9
link-alex
force-pushed
the
fix-warning-without-stack-ie9
branch
from
c536a4e
to
61f5965
Compare
|
Thanks! |
| break; | ||
| default: | ||
| throw new Error( | ||
| 'warningWithoutStack() currently supports at most 8 arguments.', |
There was a problem hiding this comment.
Why we need to do this again? I mean, we have done the args.length > 8 check, we won't reach this branch
There was a problem hiding this comment.
Not really necessary. Just left here for exhaustiveness of switch. It should never happen.
|
@Kovensky, @link-alex This might be better to remove the switch, based on your idea: const message = 'Warning: ' + format;
const args = [message].concat(args.map(item => '' + item));
console.error.apply(null, args);PS. I think the correct syntax for the craziness would be const arrayArgs = ['message', 'a', 'b', 'c'];
const thisArg = null;
Function.prototype.apply.apply(console.error, [thisArg, arrayArgs]); |
* Fix warning without stack for ie9 Where console methods like log, error etc. don't have 'apply' method. Because of the lot of tests already expect that exactly console['method'] will be called - had to reapply references for console.error method facebook#13610 * pass parameters explicitly to avoid using .apply which is not supported for console methods in ie9 * Minor tweaks
* Fix warning without stack for ie9 Where console methods like log, error etc. don't have 'apply' method. Because of the lot of tests already expect that exactly console['method'] will be called - had to reapply references for console.error method facebook#13610 * pass parameters explicitly to avoid using .apply which is not supported for console methods in ie9 * Minor tweaks
Where console methods like log, error etc. don't have 'apply' method.
Because of the lot of tests already expect that exactly console['method']
will be called - had to reapply references for console.error method
#13610