[strict-component-boundaries] Exclude fixtures folder #117

alexandcote · 2018-07-19T18:34:08Z

We now exclude the fixtures folder. Maybe we should exclude everything inside tests instead?

cartogram

@alexandcote thanks for working on this! The test looks good, but what about adding a check for imports with json extensions instead? We are never going to want this rule to flag json imports because they will never be a component.

cartogram · 2018-07-19T20:30:51Z

lib/rules/strict-component-boundaries.js

-          (hasAnotherComponentInPath(pathParts) && pathParts.length > 1) ||
-          (hasComponentDirectoryInPath(pathParts) && pathParts.length > 2)
+          isANotFixtureDirectory(pathParts) &&
+          ((hasAnotherComponentInPath(pathParts) && pathParts.length > 1) ||


isANotFixtureDirectory => isNotAFixtureDirectory

alexandcote · 2018-07-20T19:17:52Z

@cartogram Do we have any other file extensions that we want to exclude, scss, json, svg, etc.. ?

cartogram · 2018-07-21T11:18:36Z

lib/rules/strict-component-boundaries.js

@@ -39,3 +40,9 @@ function hasComponentDirectoryInPath(pathParts) {
 function hasAnotherComponentInPath(pathParts) {
  return Boolean(pathParts.filter((part) => part === pascalCase(part)).length);
 }
+
+function isASupportedFileType(pathParts) {


isASupportedFileType => isSupportedFileType

cartogram · 2018-07-21T11:19:39Z

lib/rules/strict-component-boundaries.js

@@ -39,3 +40,9 @@ function hasComponentDirectoryInPath(pathParts) {
 function hasAnotherComponentInPath(pathParts) {
  return Boolean(pathParts.filter((part) => part === pascalCase(part)).length);
 }
+
+function isASupportedFileType(pathParts) {
+  const unSupportedType = ['.json', '.svg', '.scss'];


Unless there is a reason not to, just keep the .json for now.

On second thought, can you add .graphql

cartogram · 2018-07-21T11:22:25Z

tests/lib/rules/strict-component-boundaries.js

+      parserOptions,
+    },
+    {
+      code: `import someThing from '../NameOfTheQuery/mock-query.scss';`,


These are also bad practice so its weird to me they are valid. As I commented above, I think we just keep the .json one to allow for fixtures but I'd rather not loosen this rule more than needed.

cartogram · 2018-07-21T11:23:44Z

lib/rules/strict-component-boundaries.js

@@ -19,8 +19,9 @@ module.exports = {
          .filter((part) => part[0] !== '.');

        if (
-          (hasAnotherComponentInPath(pathParts) && pathParts.length > 1) ||
-          (hasComponentDirectoryInPath(pathParts) && pathParts.length > 2)
+          isASupportedFileType(pathParts) &&


isASupportedFileType => isSupportedFileType

cartogram · 2018-07-21T11:26:18Z

@alexandcote LGTM, did you want to do a patch release?

lemonmade · 2018-07-22T03:14:24Z

IMO excluding the fixtures folder is more appropriate here. Some components could easily have assets we don’t want people reaching for (i.e., our emoji picker has a json file for th emoji definition; components have their own private sass files, etc)

cartogram · 2018-07-22T22:24:50Z

@lemonmade okay, that makes sense to me... didn't realize there were other places where we would be storing json next to a component.

alexandcote · 2018-07-25T14:23:36Z

@lemonmade / @cartogram 👀

lemonmade · 2018-07-25T14:33:02Z

lib/rules/strict-component-boundaries.js

@@ -39,3 +40,7 @@ function hasComponentDirectoryInPath(pathParts) {
 function hasAnotherComponentInPath(pathParts) {
  return Boolean(pathParts.filter((part) => part === pascalCase(part)).length);
 }
+
+function isNotAFixtureDirectory(pathParts) {
+  return Boolean(pathParts.filter((part) => part === 'fixtures').length === 0);


No need for the Boolean()

lemonmade · 2018-07-25T14:33:57Z

tests/lib/rules/strict-component-boundaries.js

@@ -25,6 +25,10 @@ ruleTester.run('strict-component-boundaries', rule, {
      code: `import {someThing} from '../OtherComponent';`,
      parserOptions,
    },
+    {
+      code: `import someThing from '../OtherComponent/fixtures/mock-query.json';`,


I still think this should be caught, but ../fixtures/OtherComponent/foo.json should not (this means reaching up into another component, and getting its fixture, which doesn't seem right).

alexandcote · 2018-07-25T22:25:50Z

Ok, I dig more into this and I found a lot of issue with this rules.

We should probably be able to include everything from an external library like:

import {Subscription} from 'apollo-client/util/Observable';

This trigger a lint error because Observable is PascalCase.

You may need a folder in the GraphQL folder to structure your queries:

import something from '../graphql/SomeMockQuery/Query.graphql'

Tests can need child component:

import MyComponent from '../components/MyComponent'
import MyComponent from '../components/MyComponent/MyComponent'

You may need types from another component :

import {MyType} from 'components/MyComponent/types'
import {MyType} from '../MyComponent/types'

You may need types from a graphql query of another component :

import {MyType} from 'components/MyComponent/types'

You may prefer to use the absolute path:

sections/MyComponent/MyComponent.tsx

import {MyComponent} from 'sections/MyComponent/components'

I only list some of them. I think it will be really hard to handle all those case.
So my question is what we really want to prevent with strict-component-boundaries

cartogram · 2018-07-25T23:58:30Z

@alexandcote We want to prevent all of those except the first one.

lemonmade · 2018-07-26T18:12:19Z

We should actually be able to solve the first one, too. Using resolve from the eslint-module-utils package to get the fully resolved path for a module (see example). That will let us tell if it is in node modules or part of the real project.

cartogram · 2018-08-02T19:04:23Z

@alexandcote let me know if you need help wrapping this up.

alexandcote · 2018-08-15T14:13:49Z

Ready for 👀

cartogram · 2018-08-21T21:02:34Z

@alexandcote additions LGTM. 🚢

alexandcote requested review from cartogram and lemonmade July 19, 2018 18:34

cartogram reviewed Jul 19, 2018

View reviewed changes

cartogram approved these changes Jul 21, 2018

View reviewed changes

alexandcote force-pushed the exclude-fixtures-strict-component-boundaries branch from 4b53a88 to fe34449 Compare July 25, 2018 14:22

lemonmade suggested changes Jul 25, 2018

View reviewed changes

cartogram mentioned this pull request Jul 31, 2018

Exclude node_modules from strict-component-imports #123

Closed

alexandcote force-pushed the exclude-fixtures-strict-component-boundaries branch from 9e87f84 to 5a262ab Compare August 15, 2018 14:09

alexandcote added 4 commits August 15, 2018 10:10

[strict-component-boundaries] Exclude fixtures folder

72a1f6d

Exclude base on file type instead.

7447532

Revert type checking instead of fixtures directory

2608ede

Rework Chris

6d7584a

alexandcote force-pushed the exclude-fixtures-strict-component-boundaries branch from 5a262ab to 6d7584a Compare August 15, 2018 14:10

Update changelog

1817f1f

alexandcote requested a review from TzviPM August 15, 2018 14:13

Merge branch 'master' into exclude-fixtures-strict-component-boundaries

ab21378

alexandcote merged commit 5b56b2e into master Aug 21, 2018

alexandcote deleted the exclude-fixtures-strict-component-boundaries branch August 21, 2018 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[strict-component-boundaries] Exclude fixtures folder #117

[strict-component-boundaries] Exclude fixtures folder #117

alexandcote commented Jul 19, 2018

cartogram left a comment •

edited

Loading

cartogram Jul 19, 2018

alexandcote commented Jul 20, 2018 •

edited

Loading

cartogram Jul 21, 2018

cartogram Jul 21, 2018

cartogram Jul 21, 2018

cartogram Jul 21, 2018

cartogram Jul 21, 2018

cartogram commented Jul 21, 2018

lemonmade commented Jul 22, 2018

cartogram commented Jul 22, 2018

alexandcote commented Jul 25, 2018

lemonmade Jul 25, 2018

lemonmade Jul 25, 2018

alexandcote commented Jul 25, 2018 •

edited

Loading

cartogram commented Jul 25, 2018

lemonmade commented Jul 26, 2018

cartogram commented Aug 2, 2018

alexandcote commented Aug 15, 2018

cartogram commented Aug 21, 2018

[strict-component-boundaries] Exclude fixtures folder #117

[strict-component-boundaries] Exclude fixtures folder #117

Conversation

alexandcote commented Jul 19, 2018

cartogram left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandcote commented Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartogram commented Jul 21, 2018

lemonmade commented Jul 22, 2018

cartogram commented Jul 22, 2018

alexandcote commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandcote commented Jul 25, 2018 • edited Loading

cartogram commented Jul 25, 2018

lemonmade commented Jul 26, 2018

cartogram commented Aug 2, 2018

alexandcote commented Aug 15, 2018

cartogram commented Aug 21, 2018

cartogram left a comment •

edited

Loading

alexandcote commented Jul 20, 2018 •

edited

Loading

alexandcote commented Jul 25, 2018 •

edited

Loading