V8 Planned Changes

[bmish]

Starting a list of my desired breaking changes, feedback welcome. Likely this release won't happen until 2022-05 or 2022-06 when we can drop Node 12/17 support, unless we come up with additional breaking changes we want to release sooner.

• Drop support for Node 12, 14, 17, 19 #373
• Strictly define Node API #374
• Change require-expect rule default option to never-except-zero #375
• feat!: Require eslint@^8.38, drop support for eslint@7.x #376
• Upgrade dependencies in preparation for major release #377
• Enable additional recommended rules:
  • (no new rules to consider yet)

Changes for future major versions:

• Enable additional recommended rules:
  • New Rule: no-invalid-names #298

For reference, V7 was release 2021-10 (CHANGELOG, discussion in #175).

[sukima]

I would like to propose that the default require-expect rule be changed from except-simple to never-except-zero.

the use of expect was originally a legacy issue and has since been superseded with better patters like assert.async() and Promises. It would be helpful that teams who prefer the expect pattern can opt-in instead of continuing to recommend a pattern that is no longer needed and mostly redundant.

[bmish]

I agree that we shouldn't unnecessarily require expect everywhere. I also use the never-except-zero setting for require-expect in my projects.

[jacobq]

QUnit's docs for expect say:

It is recommended to test asynchronous code with assert.step() or assert.async() instead.

so I appreciate the move to "Change require-expect rule default to never-except-zero"
but I have a question about that. Do async tests still need to tell QUnit when they're done or does returning a promise (/ using an async function) do this automatically?

  // this is getting flagged by `require-expect` (technically correct but missing the point, I think)
  test('it does some async things just fine', async function (assert) {
    const done = assert.async(); // <== do I still need to do this? seems to work without it
    // ... do the testing ...
    done();
  })

In my case, the test is reading in a CSV file and performing an assert.strictEquals on every field, so there a zillions of them (and it seems silly/brittle to try to count them). Does it not make sense to have a default/recommended rule that doesn't seem to match QUnit's own recommendation or am I missing something?

[platinumazure]

@jacobq If you return a promise or use an async function, QUnit will wait for that promise to be resolved. So you shouldn't need to use assert.async in that case.

[jacobq]

@jacobq If you return a promise or use an async function, QUnit will wait for that promise to be resolved. So you shouldn't need to use assert.async in that case.

Thanks, that's sort-of what I thought (hadn't had to use async/done for a while), but I got confused about why the rule was telling me I needed expect. I've turned it off in my config now and am glad the default is getting changed.

[platinumazure]

@bmish This is becoming relatively urgent for us as GitHub Actions CI are now failing: https://github.com/platinumazure/eslint-plugin-qunit/actions/runs/5391205279/jobs/9787683963

Researched the issue and found this: https://stackoverflow.com/a/76255847

I think dependabot upgraded our lockfile to version 3 and that has broken our CI on npm 12 and 14. 12 is way past EOL and 14 recently hit EOL. So I think it's time to cut a major release here.

Is your list of proposed breaking changes still reasonably accurate? Do you have anything you want to add?

[bmish]

@platinumazure I agree it's a good time for a release. I just opened up PRs for the 3 breaking changes I would like to get included. If you want to review and approve those, then one of us can conduct the major release now (I'm happy to if that helps).

[platinumazure]

@bmish Thanks! Only other thing I want to do is upgrade the other dependencies (which I started doing locally on top of my own Node version breaking change but I will take yours and rebase). I'll merge your 3 PRs, finish my changes, then push my own PR-- after that we can do the major release. Sound good?

Thanks for your help!

[bmish]

Sounds good, please go ahead and merge my PRs and do all your dependency upgrades.

[platinumazure]

@bmish I've created #376 and #377 and requested your review on each. If you have no concerns, I think we could release after they are merged.

[bmish]

@platinumazure approved. Feel free to conduct the major release. If you'd like me to do it, I'm happy to.

[platinumazure]

@bmish Thanks! I've merged everything that is ready to merge.

I'm having trouble with the release. I followed the instructions in RELEASE.md but it generated a patch bump rather than a major bump. When you have time, could you take a look and see if we need to change any configuration for release-it? Thanks!

[bmish]

@platinumazure I opened #380 which should improve the automatic version numbers and the quality of the changelog generated.

[platinumazure]

@bmish I've cut a release and it looks good to me. Mind checking on your end? If all looks good, feel free to close the issue or leave a comment. Thanks for your help today!

[bmish]

Looks great! Thanks! https://github.com/platinumazure/eslint-plugin-qunit/releases/tag/v8.0.0

[platinumazure]

Thanks again @bmish for all your help!