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

@edx/frontend-build v12+ is not properly linting with eslint #114

Closed
adamstankiewicz opened this issue Jul 19, 2022 · 2 comments
Closed
Assignees
Labels
bug Report of or fix for something that isn't working as intended

Comments

@adamstankiewicz
Copy link
Member

adamstankiewicz commented Jul 19, 2022

v12.0.0 of @edx/frontend-build upgrades ESLint 6 -> ESLint 8, however it appears this upgrade seems to break linting altogether, giving a false indication that there are no ESLint errors after upgrading, even when intentionally causing an ESLint error.

As a result, consumers using v12 of @edx/frontend-build currently have no linting in place, even though the eslint command returns no errors.

This can be seen in the example app within @edx/frontend-build itself. On v12 of @edx/frontend-build, the lint command (i.e., cd example && npm run lint) passes, however in VSCode, there is the following error when trying to render JSX in App:

image

Adding requireConfigFile: true in .eslintrc.js resolves the above error, but introduces another:

image

To resolve this error, I had to add babelOptions to the parserOptions so that ESLint knows how to parse JS/JSX files properly:

parserOptions: {
  requireConfigFile: true,
  babelOptions: {
    configFile: path.resolve(__dirname, './babel.config.js'),
  },
},

By doing the above changes, ESLint in the example app (VSCode and npm run lint) works as expected again, catching intentionally introduced ESLint errors.

However, by needing the .eslintrc.js file to point to a babel.config.js file might cause complications here, since consumers can override this babel configuration, in which case we'd want the babelOptions.configFile to point to the consumer's overridden Babel configuration, not the default babel.config.js file.

On v11 of @edx/frontend-build, intentionally causing an ESLint error in the example app, works as expected.

Upgrading @edx/frontend-build to v11 in frontend-app-learner-portal-enterprise (source) shows new ESLint errors related to hooks, but upgrading to v12 without first fixing any of these issues, incorrectly passes ESLint.

@adamstankiewicz adamstankiewicz added the bug Report of or fix for something that isn't working as intended label Jul 19, 2022
@adamstankiewicz adamstankiewicz moved this to Todo in FED-BOM Jul 19, 2022
@BilalQamar95 BilalQamar95 moved this from Todo to In Progress in FED-BOM Jul 21, 2022
@BilalQamar95 BilalQamar95 self-assigned this Jul 21, 2022
@BilalQamar95 BilalQamar95 moved this from In Progress to Done in FED-BOM Jul 22, 2022
@BilalQamar95 BilalQamar95 moved this from Done to In Progress in FED-BOM Jul 22, 2022
@BilalQamar95 BilalQamar95 moved this from In Progress to Done in FED-BOM Jul 25, 2022
@arbrandes
Copy link

@BilalQamar95, does openedx/frontend-build#255 fix this?

@BilalQamar95
Copy link

@BilalQamar95, does openedx/frontend-build#255 fix this?

@arbrandes Yes, that fixed the issue. This is now resolved

Repository owner moved this from Closed to In progress in Frontend Working Group Nov 1, 2022
@arbrandes arbrandes moved this from In progress to Closed in Frontend Working Group Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
Status: Closed
Development

No branches or pull requests

3 participants