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

fix: add cjs override for esm projects (fixes #59) #63

Merged
merged 4 commits into from
Jun 14, 2023
Merged

fix: add cjs override for esm projects (fixes #59) #63

merged 4 commits into from
Jun 14, 2023

Conversation

aladdin-add
Copy link
Member

see the discussion: #59 (comment)

@eslint-github-bot eslint-github-bot bot added the bug Something isn't working label May 19, 2023
@aladdin-add aladdin-add changed the title fix: add globals: module in generated .eslintrc.js (fixes #59) fix: add /* globals module*/ in generated .eslintrc.js (fixes #59) May 19, 2023
@aladdin-add aladdin-add requested a review from a team May 19, 2023 03:48
@@ -83,7 +83,7 @@ async function writeYAMLConfigFile(config, filePath) {
async function writeJSConfigFile(config, filePath) {
debug(`Writing JS config file: ${filePath}`);

const stringifiedContent = `module.exports = ${stringify(config, { cmp: sortByKey, space: 4 })}\n`;
const stringifiedContent = `/* globals module */\nmodule.exports = ${stringify(config, { cmp: sortByKey, space: 4 })}\n`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add /* globals module */ unconditionally, then the user will get the A config file was generated, but the config file itself may not follow your linting rules. message we're trying to avoid, this time if they choose What type of modules does your project use? · commonjs because of the no-redeclare rule.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time understanding what you're saying here.

The error message is always output for JS config files, I don't think the point was to eliminate that message. The point was to eliminate a linting error.

Are you saying something else is already declaring module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's happening when

  • globals: {module: true} or env: { commonjs: true}
  • rule no-redeclare was enabled

In this case, it will introduce a new linting error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see a few options to resolve it:

  1. adding /*eslint env: node*/, the drawback is that env will go away in the new config.
  2. adding /* eslint-disable-next-line no-undef*/. the comment will be removed by the fixer if the gloabal module has been declared in the eslintrc, e.g. eslint-config-airbnb.
  3. adding a config item for the .eslintrc.cjs, something like
module.exports = {
  ...,
  overrides: [{files: [".eslintrc.cjs"], globals: {"module": true}}],
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solution 3 with overrides sounds good to me.

@aladdin-add aladdin-add changed the title fix: add /* globals module*/ in generated .eslintrc.js (fixes #59) fix: add cjs override for esm projects (fixes #59) Jun 2, 2023
@aladdin-add aladdin-add force-pushed the issue59 branch 2 times, most recently from 8c285e1 to e84bfaa Compare June 2, 2023 04:09
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Leaving open for @nzakas to verify.

@aladdin-add aladdin-add requested a review from nzakas June 7, 2023 10:21
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

@nzakas nzakas merged commit 2568629 into main Jun 14, 2023
14 checks passed
@nzakas nzakas deleted the issue59 branch June 14, 2023 15:46
@github-actions github-actions bot mentioned this pull request Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants