selaux / eslint-plugin-filenames

Eslint plugin to check filenames.
318 stars 35 forks source link

version 1.3.0 should have been a breaking change #30

Closed brokenmass closed 6 years ago

brokenmass commented 6 years ago

The changes introduced in the latest (1.3.0) package release should have determined a major version bump as they 'break' the current validation.

Our build now fails as

// SomeContainer.js
import React from 'react';
import {withRouter} from 'react-router-dom';

class SomeContainer extends React.Component {
  // stuff
}

export default withRouter(SomeContainer)

now fails (as withRouter name does not match SomeContainer)

I really don't think https://github.com/selaux/eslint-plugin-filenames/pull/29 was a good idea as it's matching the name of the called function and does not consider, instead, the name of the value returned by that called function (that eslint cannot know as it would need some kind of dynamic code analysis), making this new rule quite annoying. The rule is not even working properly as it doesn't understand chained function and because of that LUCKILY connect(mapStateToProps, mapDispatchToProps)(SomeContainer) still pass the tests. It's my opinion that export default someAugmentingFunction(SomeContainer) is semantically clear enough and that should be considered valid by this rule.

Excluding my personal opinion, this rule had to be considered a breaking change, or, alternatively a toggle to disable the function exports name check, would have been introduced and disabled by default.

craigbeck commented 6 years ago

:+1: 1.3.0 breaks here as well with decorated exports

/Users/craigbeck/dev/premise-frontend-v3/packages/webapps/campaign-webapp/src/form/component/FormsList.react.js
  3:1  error  Filename 'FormsList' must match the exported name 'CSSModules'  filenames/match-exported

We use CSSModules extensively and hav many components decorated like this:

export default CSSModule(FileList, styles);

where this is in FileList.react.js

This is a pretty common pattern and I would expect it to be supported in some way

selaux commented 6 years ago

Yes that was an oversight on my part. A solution that I see would be to make it configurable which methods for export detection are used. In the meanwhile I will publish a version that disables the new feature by default in the next days...

craigbeck commented 6 years ago

thanks @selaux

selaux commented 6 years ago

1.3.1 is released, please test...

craigbeck commented 6 years ago

It seem there are not options for all calls to getExportedName

> eslint . --cache --cache-file eslint-cache/eslintcache

Cannot read property '2' of undefined
TypeError: Cannot read property '2' of undefined
    at getNodeName (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint-plugin-filenames/lib/common/getExportedName.js:10:16)
    at getExportedName (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint-plugin-filenames/lib/common/getExportedName.js:33:20)
    at Program (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint-plugin-filenames/lib/rules/match-regex.js:28:39)
    at listeners.(anonymous function).forEach.listener (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint/lib/util/safe-emitter.js:47:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint/lib/util/safe-emitter.js:47:38)
    at NodeEventGenerator.applySelector (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint/lib/util/node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint/lib/util/node-event-generator.js:280:22)
    at NodeEventGenerator.enterNode (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint/lib/util/node-event-generator.js:294:14)
    at CodePathAnalyzer.enterNode (/Users/craigbeck/dev/premise-frontend-v3/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)

As far as I can tell this is happening in a file w /* eslint-disable */ with an export like

module.exports = {
  process: (src, filename, config, transformOptions) => {
    const parts = filename.split('/');
    /* more code */
  }
}
selaux commented 6 years ago

Crap. I released the next version, that should fix it.

craigbeck commented 6 years ago

Looks good @selaux

eslint passes now, thanks