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

(WIP) Add eslint config and test target, plus lint and bug fixes #57395

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@JohnHeitmann
Copy link
Contributor

JohnHeitmann commented Jan 7, 2019

This change:

  • Fixes some basic lints
  • Switches var usage to let where applicable
  • Adds eslint as an optional test in the rustdoc-js target
  • Adds an eslint installer to ci
  • Fixes a bug identified by the lint

This is a continuation of #56956 , which I botched with some bad git pushes. The lint and var fixes are unchanged from that PR, and the other changes are responsive to comments in that PR.

Marked WIP since I haven't been able to test the travis bits end-to-end in my own repository. I'm hoping the CI build triggered by this run will pick up the pending changes and give me a look at it running.

r? @QuietMisdreavus

JohnHeitmann added some commits Dec 28, 2018

Fix all non-bug lints
This change fixes all non-bug lints. A few lint rules are tweaked, and a few code lines are tweaked (almost all dead code elimination).

Also, update the rustdoc-js tester in anticipation of let/const statements.

JohnHeitmann added some commits Jan 5, 2019

Delete buggy code that is probably dead
i_e is not defined, so in some cases this block cannot work. Furthermore, the toggle-wrapper is added after this block runs, so the hasClass(toggle-wrapper) check never seems to run. This appears to be dead code.

@JohnHeitmann JohnHeitmann force-pushed the JohnHeitmann:lints-submit-again branch from 31d83e9 to 78ecdb9 Jan 7, 2019

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Jan 7, 2019

Travis did pass, however this is what it did:

[01:34:10] No nodejs found, skipping "src/test/rustdoc-js" tests
[01:34:10] No eslint found, skipping js linting

So: good that the new test harness ran, bad that the new logic didn't run, but good that it's in good company with nodejs. At this point I think I need a little help from someone savvy in the build system to point me in the right direction.

I either need a pointer on how to get the old nodejs working on travis, or a pointer on how to get my personal travis instance working so I can independently track this down. The error I'm running in to in my own travis run is:

"No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself."

during stage 0 crate artifacts compilation.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 7, 2019

Based on other history with the rustdoc-js tests, i think the macOS builders and maybe appveyor have node available? We'd have to configure those builders to also install eslint. @kennytm or @Mark-Simulacrum may know more about setting that up.

For the purposes of testing, you can temporarily modify travis.yml to switch one of those builders to pull_request = true OR branch = auto instead of just the auto branch check. That will make it run without having to take up space in the testing queue.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 7, 2019

Also cc @GuillaumeGomez for the continuation of this JavaScript PR.

@@ -649,8 +648,6 @@ if (!DOMTokenList.prototype.remove) {
if (lev.pos !== -1) {
elems.splice(lev.pos, 1);
lev_distance = Math.min(lev.lev, lev_distance);
total += lev.lev;

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 8, 2019

Member

Nice catch!

function buildToggleWrapper(e) {
if (hasClass(e, "autohide")) {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 8, 2019

Member

Why removing this code?

This comment has been minimized.

@JohnHeitmann

JohnHeitmann Jan 8, 2019

Contributor

i_e is not defined, so the innermost block can never run without crashing. In looking into the problem I found that in practice I could never trigger the outer code block no matter what my autohide settings were. Looking at the code again, the only thing that sets a toggle-wrapper class is added after this block runs, so unless this block is run twice on the same element (this is the case I'm worried I missed), this code is dead.

If you can provide a repro for how to run this I can refine the bug fix to be more targetted.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 8, 2019

For eslint installation, you just need to check if node is available. If not, you don't run the tests. Also, if someone could confirm that let is working on iOS, that'd be helpful. :)

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Jan 8, 2019

According to https://caniuse.com/#search=let let is supported since iOS 10.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 8, 2019

@kennytm Which iphone version is it on?

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Jan 8, 2019

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 8, 2019

For context, my up-to-date iPhone is on iOS 12.1.2, so iOS 10 would have launched almost 2.5 years ago.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 8, 2019

2 years and a half seems pretty recent to me. :-/

@memoryruins

This comment has been minimized.

Copy link
Contributor

memoryruins commented Jan 8, 2019

For additional context, the previous 7-8 years of iPhone devices support iOS 10 (iPhone 4s and before are not supported). All supported devices receive update notifications around the same time.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 8, 2019

Btw, do we have some numbers for devices going on rust docs and docs.rs? cc @onur

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 9, 2019

I can look at our CDN for some statistics for doc.rust-lang.org and docs.rs; if you have anything specific in mind that'd be great (e.g., if we specifically want OS, or just browser, if we care what version of browser, etc.)

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Jan 9, 2019

I test on iOS. The only platform I don't have test coverage on is UC Android. If someone has a walkthrough on how to run that (even if it's "Spend $50 on device X") please let me know. I spent a long time fighting Android emulators trying to get it to run.

Edit: To clarify in case I over-promised: I test before release; I haven't tested on iOS here yet, but will. For a change like this where I'm not too worried about browser-specific breakage I wait to test until everything else is set since it costs some time to deploy and browser hop.

If I can get the node-dependent target running on the linux instances would that be desirable? I think I'm close that that.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 9, 2019

@Mark-Simulacrum: I'd like to know the % of old browsers mainly.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 9, 2019

These are statistics for the past ~2 months (since November 11th). Let me know if I can offer any further insight.

doc.rust-lang.org:

image

docs.rs:
image

www.rust-lang.org:
image

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Jan 9, 2019

Is "Chrome Mobile 41" for docs.rs a typo?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 9, 2019

These are direct screenshots from Cloudfront, so presumably not. I agree that it's a bit odd, but there's perhaps an older version of Android that has that as the maximum...

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 10, 2019

The "Others" numbers are very high which worries me about switching to more recent JS features... We should maybe officialize what JS version we want to support (the current version minus 5 years?).

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 10, 2019

I would assume that policy would fall out of the "Tiered browser support" RFC:

Supported browsers

Goal: Ensure functionality of our web content for 80% of users.

Browsers:

last 2 Chrome versions
last 1 Firefox version
Firefox ESR
last 1 Safari version
last 1 iOS version
last 1 Edge version
last 1 UCAndroid version
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 10, 2019

Ah right. I was against this choice because it literally excludes a fifth of our users but that's another debate. The "others" confirm this thought though. Should we exclude them for not so much gain (let over var doesn't change much in itself even if let is much nicer to use) or not?

For me, documentation should remain accessible to the most, targetting browsers this recent would exclude them. We don't need fancy things, it's "just" doc after all.

@JohnHeitmann

This comment has been minimized.

Copy link
Contributor

JohnHeitmann commented Jan 11, 2019

I'm planning on taking a few weeks break from the travis work on this PR before picking it up again. In the meantime I'll keep an eye out on lints in my personal branch and poke folks if things go awry. I'll also pull out the bug fix from here and push that separately. Normally I'd just close this out right now so it doesn't crud up the PR list, but the browser support conversation is valuable and I don't want to truncate it. I'll keep this open until the conversation quiesces.

My 2¢ is that a browser support list is pretty anemic without a commensurate browser test plan. If a browser is listed as officially supported, then it should be at least manually tested on a couple handfuls of representative pages before each release. This will make each supported browser be more costly, which is probably healthy.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 11, 2019

I wrote a PR to start all that with chrome testing. Unfortunately, it's been postponed until the infra team has enough time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment