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

some "where" rules do not notify warning/error on text editors #26

Closed
cirocfc opened this issue Jan 9, 2020 · 8 comments
Closed

some "where" rules do not notify warning/error on text editors #26

cirocfc opened this issue Jan 9, 2020 · 8 comments
Labels
bug Something isn't working question Further information is requested

Comments

@cirocfc
Copy link

cirocfc commented Jan 9, 2020

Issue

Running the command eslint . all the rules are notified properly. But when using ESLint on text editors such as VSCode and Atom for instance, some warnings/erros are not notified:

"where" rule:

// not notified cases
export const a = () => {};

// or

const b = () => {};
b();

// notified
export default () => {};

I changed some tests, adding this cases:

QUnit.test( "WHERE (global, default): violating const", function test(assert){
	var code = `
		const f = x => y;
	`;

	var results = eslinter.verify( code, linterOptions.whereGlobalDefault );
	var [{ ruleId, messageId, } = {},] = results || [];

	assert.expect( 3 );
	assert.strictEqual( results.length, 1, "only 1 error" );
	assert.strictEqual( ruleId, "@getify/proper-arrows/where", "ruleId" );
	assert.strictEqual( messageId, "noGlobal", "messageId" );
} );

✅ The test above passes but this warning/error is not notified on the editor.

QUnit.test( "WHERE (export): violating const", function test(assert){
	var code = `
		export const a = () => {};
	`;

	var results = eslinter.verify( code, linterOptions.whereExport );
	// HERE THE RESULTS VARIABLE IS AN EMPTY ARRAY []
	var [{ ruleId, messageId, } = {},] = results || [];

	assert.expect( 3 );
	assert.strictEqual( results.length, 1, "only 1 error" );
	assert.strictEqual( ruleId, "@getify/proper-arrows/where", "ruleId" );
	assert.strictEqual( messageId, "noExport", "messageId" );
} );

❌ The test above breaks because results variable is an empty array. But this case is notified while running on CLI.


Could someone help me understand what is the problem in these cases?

I was trying to add an export-only-named-function (do-not-export-arrow-functions) rule to my project and found this awesome ESLint plugin that would fit perfectly for my scenario. The problem is that I cannot add it if other developers cannot see it at development time. It would only notify on CI by the time they open a Pull Request, and it would be a bad experience to find the log and come back to the code to fix a linter issue 😞

More info:

Tested editors: VSCode and Atom

OS: Mac OS X Catalina (10.15.2)

Node: v12.13.0
npm: 6.12.0

My project is using ESLint v5.16.0

ESLint config:

// .eslintrc.js
module.exports = {
  root: true,
  env: {
    browser: true,
    es6: true,
    jest: true,
  },
  parserOptions: {
    parser: 'babel-eslint',
  },
  plugins: [
    '@getify/proper-arrows',
  ],
  rules: {
    '@getify/proper-arrows/where': 'error',
  },
};
@getify
Copy link
Owner

getify commented Jan 9, 2020

This plugin itself should not have any different behavior depending on what ESLint env it is run in. The only explanation I can think of is either different configuration being applied in these different envs, or (more likely!) ESLint itself is a different version in these different envs, and therefore it produces a different parse tree for the plugin to inspect.

@cirocfc
Copy link
Author

cirocfc commented Jan 10, 2020

That's what I thought! I checked the ESLint version on my project, it is using 5.16.0. The ESLint installed version for eslint-plugin-proper-arrows is also using 5.16.0. So far so good.

On the other hand, VSCode and its extensions are all updated, so they are using an ESLint version greater than version 6 according to their Github repo.

When the problem first occurred I thought it was the versions, so I updated my project to use version 6.6.0 (just for testing, cause I cannot change it until a plugin is also updated) and the CLI continued to report errors on "where" rule only using export default () => { }; but not when using export const a = () => { };. The editor did report the first one but not the second one as well. So now the results are the same, but the export const a = () => { }; case is not being notified.

Maybe something changed from version 5 to 6 on ESLint and affected how this plugin notifies the "where" rules?

By using the test case I could get an empty array for the case above, so it would never be caught on this if statement and would not be notified.

@getify
Copy link
Owner

getify commented Jan 23, 2020

So, to restate, is the issue that Babel has changed its representation (in an AST sense) of export const a = .. from version 5 to 6, so that my plugin no longer recognizes those?

@getify getify added bug Something isn't working question Further information is requested labels Jan 23, 2020
@cirocfc
Copy link
Author

cirocfc commented Jan 24, 2020

So, to restate, is the issue that Babel has changed its representation (in an AST sense) of export const a = .. from version 5 to 6, so that my plugin no longer recognizes those?

I think you meant ESLint instead of Babel, right? In this case the answer is yes. What I am lead to believe is that ESLint changed how it reports the code representation.

I made an extremely simple sample of my setup, just so you can be sure I am not messing it up. And if that is the case, please forgive me. This is the repo https://github.com/cirocfc/arrow-lint-example.

Note that I intentionally duplicated my functions inside index.js file so the first part could be reported by your plugin while the second was supposed to work fine.

@getify
Copy link
Owner

getify commented Apr 29, 2020

I just upgraded to ESLint 6.8.0, and the test suite fully passes. Am I missing a test case that should be failing on 6.x?

@cirocfc
Copy link
Author

cirocfc commented Apr 29, 2020

Hey Kyle!

Thanks for the updates. I just updated my project with the new version of your plugin and ESLint to v6 and it seems that the version related erros are all gone.

The only case that I noticed is that when a function is declared using an arrow and it is exported. In this case the plugin does not notify if global flag is false. It should be notified by export flag, right?

For instance:

// some_code.js

// global as false: not notified
export const a = () => { console.log('hello!'); };
// export const is not covered, but export default is

// global as false: notified
export default () => { console.log('hello!'); };
// export default is covered, but not export const

// notified with global as true as it is a top level arrow
// but not because of export flag
export const a = () => { };

This is the config that I am testing it on:

// .eslintrc.js
module.exports = {
  root: true,
  env: {
    node: true,
    browser: true,
    es6: true,
    jest: true,
  },
  extends: [
    // I still have to refactor some files in order to enable the full plugin
    // For now I am using just `export` and `property`
    // 'plugin:@getify/proper-arrows/getify-says',
  ],
  parserOptions: {
    parser: 'babel-eslint',
  },
  plugins: [
    '@getify/proper-arrows',
  ],
  rules: {
    '@getify/proper-arrows/where': ['error', {
      'global': false,
      'property': true,
      'export': true,
      'trivial': false
    }]
  },
};

I updated the example code btw

@getify
Copy link
Owner

getify commented Jul 15, 2020

Sorry this took so long to get around to fixing, but should be fixed now, released as v10.0.0. Thanks!

@getify getify closed this as completed Jul 15, 2020
getify added a commit that referenced this issue Jul 15, 2020
…rt on 'export var x = v => v' (named-declaration exports) form, closes #26
@cirocfc
Copy link
Author

cirocfc commented Jul 16, 2020

I'm truly grateful Kyle!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants