jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
9k stars 2.77k forks source link

[Bug]: Getting several "Unknown property 'someprop' found react/no-unknown-property" but only on Linux not macOS #3517

Closed jamesmortensen closed 1 year ago

jamesmortensen commented 1 year ago

Is there an existing issue for this?

Description Overview

I observed on GitHub Actions, the build process would fail due to react/no-unknown-property errors, even if CI is explicitly set to failse. However, the same code on the local machine (macOS M1 v13.0.1) would build with warnings (and the warnings were related to other issues, not the no-unknown-property errors).

In order to debug the issue further, I booted up an Ubuntu 22.04 instance and ran the build process with the same code and same Node version, and I see the same result as on GitHub Actions. Thus, I've ruled out the issue being related to the CI environment variable or something specific to GitHub Actions. It seems as if the rules are applied differently on Linux vs macOS.

The error is generated by just running your standard scripts/build.js script:

$ CI=false && GENERATE_SOURCEMAP=true node scripts/build.js

Or variations of the above command, like:

$ export CI=false
$ GENERATE_SOURCEMAP=true node scripts/build.js

scripts/build.js:

'use strict';

// Do this as the first thing so that any code reading it knows the right env.
process.env.BABEL_ENV = 'production';
process.env.NODE_ENV = 'production';

// Makes the script crash on unhandled rejections instead of silently
// ignoring them. In the future, promise rejections that are not handled will
// terminate the Node.js process with a non-zero exit code.
process.on('unhandledRejection', err => {
  throw err;
});

// Ensure environment variables are read.
require('../config/env');

const path = require('path');
const chalk = require('react-dev-utils/chalk');
const fs = require('fs-extra');
const bfj = require('bfj');
const webpack = require('webpack');
const configFactory = require('../config/webpack.config');
const paths = require('../config/paths');
const checkRequiredFiles = require('react-dev-utils/checkRequiredFiles');
const formatWebpackMessages = require('react-dev-utils/formatWebpackMessages');
const printHostingInstructions = require('react-dev-utils/printHostingInstructions');
const FileSizeReporter = require('react-dev-utils/FileSizeReporter');
const printBuildError = require('react-dev-utils/printBuildError');

const measureFileSizesBeforeBuild =
  FileSizeReporter.measureFileSizesBeforeBuild;
const printFileSizesAfterBuild = FileSizeReporter.printFileSizesAfterBuild;
const useYarn = fs.existsSync(paths.yarnLockFile);

// These sizes are pretty large. We'll warn for bundles exceeding them.
const WARN_AFTER_BUNDLE_GZIP_SIZE = 512 * 1024;
const WARN_AFTER_CHUNK_GZIP_SIZE = 1024 * 1024;

const isInteractive = process.stdout.isTTY;

// Warn and crash if required files are missing
if (!checkRequiredFiles([paths.appHtml, paths.appIndexJs])) {
  process.exit(1);
}

const argv = process.argv.slice(2);
const writeStatsJson = argv.indexOf('--stats') !== -1;

// Generate configuration
const config = configFactory('production');

// We require that you explicitly set browsers and do not fall back to
// browserslist defaults.
const { checkBrowsers } = require('react-dev-utils/browsersHelper');
checkBrowsers(paths.appPath, isInteractive)
  .then(() => {
    // First, read the current file sizes in build directory.
    // This lets us display how much they changed later.
    return measureFileSizesBeforeBuild(paths.appBuild);
  })
  .then(previousFileSizes => {
    // Remove all content but keep the directory so that
    // if you're in it, you don't end up in Trash
    fs.emptyDirSync(paths.appBuild);
    // Merge with the public folder
    copyPublicFolder();
    // Start the webpack build
    return build(previousFileSizes);
  })
  .then(
    ({ stats, previousFileSizes, warnings }) => {
      if (warnings.length) {
        console.log(chalk.yellow('Compiled with warnings.\n'));
        console.log(warnings.join('\n\n'));
        console.log(
          '\nSearch for the ' +
            chalk.underline(chalk.yellow('keywords')) +
            ' to learn more about each warning.'
        );
        console.log(
          'To ignore, add ' +
            chalk.cyan('// eslint-disable-next-line') +
            ' to the line before.\n'
        );
      } else {
        console.log(chalk.green('Compiled successfully.\n'));
      }

      console.log('File sizes after gzip:\n');
      printFileSizesAfterBuild(
        stats,
        previousFileSizes,
        paths.appBuild,
        WARN_AFTER_BUNDLE_GZIP_SIZE,
        WARN_AFTER_CHUNK_GZIP_SIZE
      );
      console.log();

      const appPackage = require(paths.appPackageJson);
      const publicUrl = paths.publicUrlOrPath;
      const publicPath = config.output.publicPath;
      const buildFolder = path.relative(process.cwd(), paths.appBuild);
      printHostingInstructions(
        appPackage,
        publicUrl,
        publicPath,
        buildFolder,
        useYarn
      );
    },
    err => {
      const tscCompileOnError = process.env.TSC_COMPILE_ON_ERROR === 'true';
      if (tscCompileOnError) {
        console.log(
          chalk.yellow(
            'Compiled with the following type errors (you may want to check these before deploying your app):\n'
          )
        );
        printBuildError(err);
      } else {
        console.log(chalk.red('Failed to compile.\n'));
        printBuildError(err);
        process.exit(1);
      }
    }
  )
  .catch(err => {
    if (err && err.message) {
      console.log(err.message);
    }
    process.exit(1);
  });

// Create the production build and print the deployment instructions.
function build(previousFileSizes) {
  console.log('Creating an optimized production build...');

  const compiler = webpack(config);
  return new Promise((resolve, reject) => {
    compiler.run((err, stats) => {
      let messages;
      if (err) {
        if (!err.message) {
          return reject(err);
        }

        let errMessage = err.message;

        // Add additional information for postcss errors
        if (Object.prototype.hasOwnProperty.call(err, 'postcssNode')) {
          errMessage +=
            '\nCompileError: Begins at CSS selector ' +
            err['postcssNode'].selector;
        }

        messages = formatWebpackMessages({
          errors: [errMessage],
          warnings: [],
        });
      } else {
        messages = formatWebpackMessages(
          stats.toJson({ all: false, warnings: true, errors: true })
        );
      }
      if (messages.errors.length) {
        // Only keep the first error. Others are often indicative
        // of the same problem, but confuse the reader with noise.
        if (messages.errors.length > 1) {
          messages.errors.length = 1;
        }
        return reject(new Error(messages.errors.join('\n\n')));
      }
      console.error('CI = ' + process.env.CI);
      if (
        process.env.CI &&
        (typeof process.env.CI !== 'string' ||
          process.env.CI.toLowerCase() !== 'false') &&
        messages.warnings.length
      ) {
        console.log(
          chalk.yellow(
            '\nTreating warnings as errors because process.env.CI = true.\n' +
              'Most CI servers set it automatically.\n'
          )
        );
        return reject(new Error(messages.warnings.join('\n\n')));
      }

      const resolveArgs = {
        stats,
        previousFileSizes,
        warnings: messages.warnings,
      };

      if (writeStatsJson) {
        return bfj
          .write(paths.appBuild + '/bundle-stats.json', stats.toJson())
          .then(() => resolve(resolveArgs))
          .catch(error => reject(new Error(error)));
      }

      return resolve(resolveArgs);
    });
  });
}

function copyPublicFolder() {
  fs.copySync(paths.appPublic, paths.appBuild, {
    dereference: true,
    filter: file => file !== paths.appHtml,
  });
}

Error Output:

Creating an optimized production build...
Failed to compile.

src/components/genericComponents/Form.js
  Line 377:25:  Unknown property 'selector' found  react/no-unknown-property

src/components/mainContainer/DateAndTime/Slots.js
  Line 164:15:  Unknown property 'staffkey' found    react/no-unknown-property
  Line 165:15:  Unknown property 'sessionkey' found  react/no-unknown-property

src/components/mainContainer/ServiceAndClassContainer/ServiceDescription.js
  Line 130:17:  Unknown property 'fillrrule' found  react/no-unknown-property
  Line 132:17:  Unknown property 'cliprrule' found  react/no-unknown-property

src/components/mainContainer/ServiceAndClassContainer/ServiceList.js
  Line 198:9:  Unknown property 'catid' found    react/no-unknown-property
  Line 199:9:  Unknown property 'catname' found  react/no-unknown-property

src/components/mainContainer/ServiceAndClassContainer/ServiceTab.js
  Line 82:7:  Unknown property 'catid' found    react/no-unknown-property
  Line 83:7:  Unknown property 'catname' found  react/no-unknown-property

Search for the keywords to learn more about each error.

Keep in mind that the error only happens on Linux (Ubuntu 22.04). On macOS, it "compiles with warnings".

Do we know what's different about the Linux environment that's causing this to fail? Thank you!

Expected Behavior

eslint-plugin-react version

v7.31.11

eslint version

7.32.0

node version

v14.21.1

ljharb commented 1 year ago

Can you provide a snippet of the jsx that these warnings are appearing on?

jamesmortensen commented 1 year ago

To move forward, I configured the react/no-unknown-property rule in .eslintrc.json as a warning:

   "react/no-unknown-property": "warn"

I can build, but this seems like a temporary patch rather than a full resolution. I believe the behavior should be the same regardless of platform. Hope this information helps. If you need anything else, please let me know!

ljharb commented 1 year ago

@jamesmortensen see https://github.com/jsx-eslint/eslint-plugin-react/issues/3517#issuecomment-1376673939

(either way, building should never be gated by linting - linting is part of tests, which is an entirely distinct thing from build)

jamesmortensen commented 1 year ago

Hi @ljharb please let me know if this is what you are looking for:

src/components/mainContainer/DateAndTime/Slots.js Line 164:15: Unknown property 'staffkey' found react/no-unknown-property Line 165:15: Unknown property 'sessionkey' found react/no-unknown-property

return slots && slots.length > 0 ? (
      <>
        <h4>{`${window.lang.book_on} ${dateTitle}`}</h4>
        <ul>
          {slots.map((slot) => (
            <li
              id={slot.id}
              data-testid={slot.id ? slot.id.toString() : ''}
              key={slot.id}
              staffkey={slot.staffKey}
              sessionkey={slot.sessionKey}
              onClick={handleSlotSelect}
            >
              {slot.value}
            </li>
          ))}
        </ul>
      </>
jamesmortensen commented 1 year ago

...building should never be gated by linting - linting is part of tests, which is an entirely distinct thing from build)

@ljharb I guess it depends on the type of linting being done. I created a linting plugin for WebdriverIO that looks for unresolved imports because require('../modulea') and require('../moduleA') will work great on Windows/macOS, but on Linux filesystems, Linux will see it as two completely distinct paths. So instead of waiting to discover the issues in the tests on the CI server, maybe even 10 minutes into the tests, we discover it immediately on the local machine before the tests even execute. It saves a ton of time and avoids confusion.

Of course, that was for WebdriverIO tests, not a React App. I'm not familiar enough with React to have strong opinions on mixing linting and building together.

Hope the jsx snippet is helpful. Please let me know if you need anything else!

ljharb commented 1 year ago

Yes, thanks. So, that's a nonstandard html attribute - staffkey and sessionkey, for example, are invalid there. you'd need to prefix it with data- to be able to use a custom attribute. Unfortunately it was a bug to allow these in past versions, that has now been fixed.

Alternatively, you can configure the eslint rule to ignore the attribute, but i'd suggest avoiding nonstandard attribute names if possible.

jamesmortensen commented 1 year ago

@ljharb Ah, makes sense about needing the data-*. Thank you. But I wonder why it only comes up as an issue on Linux?

ljharb commented 1 year ago

That is very strange indeed. The only differences between platforms should be based on file paths and/or case sensitivity. Are you sure node_modules is the same on both machines?

jamesmortensen commented 1 year ago

The macOS node_modules won't be compatible with Linux if they contain any platform-specific binaries, so I only copied the source code (including package.json) to the Linux instance and then did an npm install on both after clearing the node_modules folder on the Mac. This ensures that the versions of all of the modules pulled on the Mac is the same as on Linux.

I did a diff of the eslint-plugin-react module on Linux with the one on macOS. They're both identical. Identical versions, identical anything...

It sounds like the long-term answer is for us to use valid attribute/property names, so I'm happy for it to be one of those "unsolved mysteries" for now. Thanks again for the help!

ljharb commented 1 year ago

Certainly! I remain very confused why the linting would differ here, assuming eslint is actually linting the files in question.

Either way, I think that the primary issue here is indeed the invalid attribute names, so I'll close.