compat-table icon indicating copy to clipboard operation
compat-table copied to clipboard

Published 20 hours ago • verified publisher icon kangax

Updates to pr1827

Open p-bakker opened this issue 1 year ago • 6 comments

Fixes issues introduced in #1827, namely:

  • runner_support.js mixing usage of the print and console.log commands, neither of which will both work on any environment
  • exceptions occurring during running of tests cluttering the stdout

Includes extracting createIterableHelper out from runner_support, as it's used by the node.js script, which must remain working on older NodeJS versions (and thus should be authored in ES3), while runner_support.js does not have that ES3 requirement (and thus can use newer syntax)

p-bakker avatar Oct 11 '23 12:10 p-bakker

It seems like you're adding try/catches in a number of places that didn't have them before; won't that mask errors in the test code we want caught?

Prior to #1827 there also were try/catched around the eval of the testcode:

  • https://github.com/compat-table/compat-table/pull/1827/files#diff-9ae6f9d9f0960230222e4bfb5f9282bcb02bd4e8e4220272bdfe4bd7ef1db3a7L88
  • https://github.com/compat-table/compat-table/pull/1827/files#diff-48b230a7f01f4595f2a9df8af1db79063c8a0e0458de67e4873877d98880cbb6L83
  • https://github.com/compat-table/compat-table/pull/1827/files#diff-3744f0f40c1d8d08319f4dce83919405e334cf138b46cc16e9848cb88a1a5489L159
  • etc.

p-bakker avatar Oct 11 '23 18:10 p-bakker

gotcha, so it's restoring them, good.

ljharb avatar Oct 11 '23 19:10 ljharb

I see the pretest stage fails due to linting. For now I fixed it to the specific file that had warnings, but going forward what's the desired way?

  1. Globally allow newer js, limit to es3 only on the data-*.js files, node.js en master.js? Or
  2. The other way around: globally limit to es3, allowing newer js on runner_support.js/build.js & the various runner .js files?

p-bakker avatar Oct 12 '23 13:10 p-bakker

Is there anything I must do now to move this PR along?

p-bakker avatar Oct 16 '23 16:10 p-bakker

I see the pretest stage fails due to linting. For now I fixed it to the specific file that had warnings, but going forward what's the desired way?

  1. Globally allow newer js, limit to es3 only on the data-*.js files, node.js en master.js? Or
  2. The other way around: globally limit to es3, allowing newer js on runner_support.js/build.js & the various runner .js files?

i would go with option 2; the default should be the safest thing.

ljharb avatar Oct 25 '23 22:10 ljharb

i would go with option 2; the default should be the safest thing.

k, that's what I figured as well and thus what I already did :-)

Just now waiting for that second approval...

p-bakker avatar Oct 26 '23 16:10 p-bakker

@chicoxyzzy any chance you can review this one?

p-bakker avatar Jun 01 '24 20:06 p-bakker