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

Fix warning without stack for ie9 #13620

Merged
merged 3 commits into from Sep 12, 2018

Conversation

@link-alex
Copy link
Contributor

@link-alex link-alex commented Sep 11, 2018

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

@pull-bot
Copy link

@pull-bot pull-bot commented Sep 11, 2018

Details of bundled changes.

Comparing: f6fb03e...f785a3d

schedule

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
schedule.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
schedule.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

Loading

const originalConsoleError = console.error;

if (typeof console.error.apply === 'undefined') {
console.error = consoleError;
Copy link
Member

@gaearon gaearon Sep 11, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@link-alex link-alex Sep 11, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member

@gaearon gaearon Sep 11, 2018

Choose a reason for hiding this comment

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

Fixed amount of args is okay. We have that in invariant.

Loading

@link-alex
Copy link
Contributor Author

@link-alex link-alex commented Sep 11, 2018

All the parameters are passed explicitly now, but result looks a bit frustrating for me.. So please have a look, thanks!

Loading

@@ -15,8 +15,6 @@
let warningWithoutStack = () => {};

if (__DEV__) {
const consoleError = Function.prototype.bind.call(console.error, console);

warningWithoutStack = function(condition, format, ...args) {
Copy link
Member

@gaearon gaearon Sep 11, 2018

Choose a reason for hiding this comment

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

Let’s assert the number of arguments isn’t higher than supported here?

Loading

Copy link
Contributor Author

@link-alex link-alex Sep 12, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member

@gaearon gaearon Sep 12, 2018

Choose a reason for hiding this comment

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

If 8 is current maximum the let’s check we have at most 8.

Loading

Copy link
Member

@gaearon gaearon Sep 12, 2018

Choose a reason for hiding this comment

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

Try rebasing to solve the CI issue

Loading

@Jessidhia
Copy link
Contributor

@Jessidhia Jessidhia commented Sep 12, 2018

This sounds a bit crazy, but I wonder if Function.prototype.apply.apply(console.error, console, args) would work 🤔

Loading

@link-alex
Copy link
Contributor Author

@link-alex link-alex commented Sep 12, 2018

@Kovensky not really as far as I can see.. but interesting idea.

Loading

@link-alex link-alex force-pushed the fix-warning-without-stack-ie9 branch from 92ec2a3 to c536a4e Sep 12, 2018
link-alex added 2 commits Sep 12, 2018
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 link-alex force-pushed the fix-warning-without-stack-ie9 branch from c536a4e to 61f5965 Sep 12, 2018
@gaearon gaearon merged commit 1b2646a into facebook:master Sep 12, 2018
1 check was pending
Loading
@gaearon
Copy link
Member

@gaearon gaearon commented Sep 12, 2018

Thanks!

Loading

break;
default:
throw new Error(
'warningWithoutStack() currently supports at most 8 arguments.',
Copy link
Contributor

@NE-SmallTown NE-SmallTown Sep 12, 2018

Choose a reason for hiding this comment

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

Why we need to do this again? I mean, we have done the args.length > 8 check, we won't reach this branch

Loading

Copy link
Member

@gaearon gaearon Sep 12, 2018

Choose a reason for hiding this comment

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

Not really necessary. Just left here for exhaustiveness of switch. It should never happen.

Loading

@lustoykov
Copy link

@lustoykov lustoykov commented Sep 12, 2018

@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]);

Loading

@gaearon gaearon mentioned this pull request Sep 13, 2018
Simek added a commit to Simek/react that referenced this issue Oct 25, 2018
* 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
jetoneza added a commit to jetoneza/react that referenced this issue Jan 23, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants