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

doc: Remove 'dnt_helper.js' #22595

Closed
wants to merge 1 commit into from
Closed

doc: Remove 'dnt_helper.js' #22595

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 30, 2018

This file is totally useless, because it's an inner-used helper that is for publishment of doc files.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows [commit guidelines]

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 30, 2018
@Trott
Copy link
Member

Trott commented Aug 30, 2018

I wonder if the file can be removed entirely and any calls to _dntEnabled() replaced with navigator.doNotTrack || window.doNotTrack. (The window check is for IE 11. If you want to be REALLY accommodating and account for IE 9 and IE 10, throw in a || navigator.msDoNotTrack.)

This file seems to exist mostly for operating systems and browsers we probably don't need to take particular care in accommodating. (Windows NT 6.1??!! Firefox 31??!!) To get an idea of just how broadly the Do Not Track API is supported on current browsers: https://caniuse.com/#search=dnt and click "Show All":

screen shot 2018-08-29 at 11 13 35 pm

Note that even for browsers that show red in that chart, it's not a problem. It just means the browsers don't support Do Not Track, which just means it will never be enabled on those browsers and that's OK. We'll just get the falsy undefined for those browsers. If the user can't set Do Not Track in their browser, then we aren't failing to respect Do Not Track.

@Trott
Copy link
Member

Trott commented Aug 30, 2018

@nodejs/website on my previous comment ^^^^^^^

@ghost ghost changed the title doc (dnt_helper.js): Remove useless link and duplicated codes doc: Remove 'dnt_helper.js' and a useless link Aug 30, 2018
@ChALkeR
Copy link
Member

ChALkeR commented Aug 30, 2018

This change will disable analytics on IE versions that break the spec and default DNT to enabled (previously the helper ignored those).

That should be fine, I guess.


I have another (unrelated here) question about analytics though. I remember claims that it will improve docs back when it was added.
Are there any issues, comments or PRs that rely on data from the analytics? Has it proven to be useful at all?

@silverwind
Copy link
Contributor

Windows NT 6.1

This is Windows 7 which still has a considerable userbase.

Are there any issues, comments or PRs that rely on data from the analytics

The question is: Who has access to this analytics account and are they actively looking at the data? If not, I suggest dropping it entirely.

@fhemberger
Copy link
Contributor

Stats for last month (65% of the Visitors use any version of Windows):

Windows 10: 70.0%
Windows 8:   5.5%
Windows 7:  24.0%
older:     < 1.0% 

Browsers used by Windows visitors:

Chrome:           81.5%
Firefox:           9.5%
Edge:              4.0%
Internet Explorer: 3.0%

@silverwind Anything else you need?

@ChALkeR
Copy link
Member

ChALkeR commented Aug 30, 2018

See also previous discussion in #6601.
/cc @phillipj

@silverwind
Copy link
Contributor

@fhemberger thanks, no it's fine to keep if we use this data. The current PR should cover IE9 and up, seems good enough and a nice simplification over the previous checks.

This file is totally useless, because it's an inner-used helper that is
for publishment of doc files.
@ghost ghost changed the title doc: Remove 'dnt_helper.js' and a useless link doc: Remove 'dnt_helper.js' Aug 30, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 31, 2018
@BridgeAR
Copy link
Member

@addaleax
Copy link
Member

addaleax commented Sep 2, 2018

Landed in ec75ba6

@addaleax addaleax closed this Sep 2, 2018
addaleax pushed a commit that referenced this pull request Sep 2, 2018
This file is totally useless, because it's an inner-used helper that is
for publishment of doc files.

PR-URL: #22595
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Sep 2, 2018
This file is totally useless, because it's an inner-used helper that is
for publishment of doc files.

PR-URL: #22595
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ghost ghost deleted the RemoveDup branch September 3, 2018 00:42
@ghost
Copy link
Author

ghost commented Sep 3, 2018

Thanks!

targos pushed a commit that referenced this pull request Sep 3, 2018
This file is totally useless, because it's an inner-used helper that is
for publishment of doc files.

PR-URL: #22595
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
This file is totally useless, because it's an inner-used helper that is
for publishment of doc files.

PR-URL: #22595
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants