Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

only call console.log when window.console exists #14007

Closed
wants to merge 1 commit into from
Closed

only call console.log when window.console exists #14007

wants to merge 1 commit into from

Conversation

lucienbertin
Copy link
Contributor

issue #14006 - window.console only exists in IE 8 & 9 when the devtools are open

issue #14006 - `window.console` only exists in IE 8 & 9 when the devtools are open
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@lucienbertin
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@@ -1,6 +1,8 @@
if (window.angular.bootstrap) {
//AngularJS is already loaded, so we can return here...
console.log('WARNING: Tried to load angular more than once.');
if (!!window.console) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not if (window.console)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's personal preference. I prefer to test if something is true rather than truthy ; !!window.console is a boolean while window.console is something

I don't think it has any impact on anything so if you prefer if (window.console) i'll change it

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are doing !! then you are still relying on console being truthy though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but my if is testing a boolean, not a something that is truthy or falsy

when i read this code

var foo, bar;
...
if (foo) { ... }
if (!!bar) { ... }

I assume foo is a boolean while bar is a something

as i said it's just personal preference


fun fact with momentjs, moment.duration(0) is truthy while being equal to 0

moment.duration(0) == 0; // true
moment.duration(0) === 0; // false
!!moment.duration(0); // true
!!0; // false, obviously

@Narretz
Copy link
Contributor

Narretz commented Feb 11, 2016

Ideally, we should have an e2e test for this. Do you want to give it a shot? There's a sample e2e test in test/e2e/sample.

@lucienbertin
Copy link
Contributor Author

I tried for the last 2 hours to make grunt work, following the angular contribute doc but it just don't want to register there is a Gruntfile.js in the dir

I dont know whats causing it, i'll let you handle the e2e test

I'll try to make it work for my next contribution


i'm running a command prompt on win 10 as admin and typing

npm install grunt grunt-cli -g
grunt package

and getting

Fatal error: Unable to find local grunt

when the dir cmd gives me

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
... ellipsis...
-a----        2/11/2016   4:36 PM           7170 gdocs.js
-a----        2/11/2016   4:36 PM          10729 Gruntfile.js
-a----        2/11/2016   4:36 PM            610 init-repo.sh
... ellipsis...

I've worked with grunt professionally for the past year, it's not my first walk around the park

@Narretz
Copy link
Contributor

Narretz commented Feb 11, 2016

Grunt must be installed without the -g flag. Only grunt-cli must be
installed globally
Am 11.02.2016 19:13 schrieb "lucienbertin" notifications@github.com:

I tried for the last 2 hours to make grunt work, following the angular
contribute doc https://docs.angularjs.org/misc/contribute but it just
don't want to register there is a Gruntfile.js in the dir

I dont know whats causing it, i'll let you handle the e2e test

I'll try to make it work for my next contribution

i'm running a command prompt on win 10 as admin and typing

npm install grunt grunt-cli -g
grunt package

and getting

Fatal error: Unable to find local grunt

when the dir cmd gives me

Mode LastWriteTime Length Name


... ellipsis...
-a---- 2/11/2016 4:36 PM 7170 gdocs.js
-a---- 2/11/2016 4:36 PM 10729 Gruntfile.js
-a---- 2/11/2016 4:36 PM 610 init-repo.sh
... ellipsis...

I've worked with grunt professionally for the past year, it's not my first
walk around the park


Reply to this email directly or view it on GitHub
#14007 (comment).

@lucienbertin
Copy link
Contributor Author

> grunt package
Loading "Gruntfile.js" tasks...ERROR
>> Error: Cannot find module 'shelljs'
Warning: Task "package" not found. Use --force to continue.

Aborted due to warnings.

I'm giving up here on the e2e test

also grunt is referenced in package.json so it should get installed when i run npm install, i shouldn't have to run npm install grunt

@Narretz
Copy link
Contributor

Narretz commented Feb 12, 2016

Sorry about that. It's possible that the latest node / npm versions are not quite compatible yet with our ancient setup.

Narretz pushed a commit to Narretz/angular.js that referenced this pull request Feb 15, 2016
issue angular#14006 - `window.console` only exists in IE 8 & 9 when the devtools are open

Closes angular#14007
Narretz pushed a commit to Narretz/angular.js that referenced this pull request Feb 15, 2016
`window.console` only exists in IE 8 & 9 when the devtools are open

Fixes angular#14006
Closes angular#14007
Closes angular#14047
@Narretz Narretz closed this in 7c60e19 Feb 16, 2016
Narretz pushed a commit that referenced this pull request Feb 16, 2016
`window.console` only exists in IE 8 & 9 when the devtools are open

Fixes #14006
Closes #14007
Closes #14047
Narretz pushed a commit that referenced this pull request Feb 16, 2016
`window.console` only exists in IE 8 & 9 when the devtools are open

Fixes #14006
Closes #14007
Closes #14047
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants