Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Fixes strict-component-boundaries #160

Merged
merged 3 commits into from
Sep 25, 2018
Merged

Conversation

cartogram
Copy link
Contributor

@cartogram cartogram commented Sep 14, 2018

This PR resolves false positives with strict-component-boundaries that were introduced with #140.

Fixes #156 #122 and #123

Problem:

The check for whether there is a component in the path was returning true if the path to a module included a pascal case word. In #140 this check changed from the ImportDeclaration source to the full path to the module on the users computer. Since Shopify is a pascal case word and in most cases there is at least one pascal case word in the full path as in Users/your-username/path, this was returning true in almost every ImportDeclaration.

Fix:

The solution was to check the import source for the component in path check rather than the full path to the module source.

🎩 Instructions:

  1. Pull this branch
  2. Install yalc: yarn add yalc -g
  3. run yalc publish
  4. cd into a project that is currently reporting false positives for this rule (such as shopify/images)
  5. install this branch using yalc: yalc add eslint-plugin-shopify
  6. confirm that the false positives are no longer invalid

@BPScott
Copy link
Member

BPScott commented Sep 14, 2018

I'm not going to pretend to know what this is doing under the hood but adding a bunch of examples to a test suite would make me feel better about it working

@cartogram cartogram force-pushed the fixt-strict-component-boundaries branch from ba669f9 to a33ac9a Compare September 15, 2018 17:32
@cartogram
Copy link
Contributor Author

@BPScott thanks for pointing that out. I added what would be a test for this fix. However, this new test is pulling from a fixture folder and we are already ignoring when fixtures is in the path so the test will already never fail.

I am going to need to go back and rework the fixtures ignore and update this PR. Changing the title to WIP in the mean time.

@cartogram cartogram changed the title Fixes strict-component-boundaries [WIP] Fixes strict-component-boundaries Sep 15, 2018
@cartogram cartogram force-pushed the fixt-strict-component-boundaries branch from a33ac9a to 9c2ab63 Compare September 15, 2018 18:24
@cartogram cartogram changed the title [WIP] Fixes strict-component-boundaries Fixes strict-component-boundaries Sep 15, 2018
@cartogram
Copy link
Contributor Author

Updates:

  1. solved fixture import so the added test is legit (it will fail without these changes)
  2. successful 🎩 in shopify/images and polaris-react

code: `import someThing from '../SomeOtherComponent/fixtures/SomeMockQuery/query.json';`,
parserOptions,
errors,
},
Copy link
Member

Choose a reason for hiding this comment

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

I feel like you are not testing the main thing you indicated this PR was to address

const pathParts = resolvedSource
? pathSegmantsFromSource(resolvedSource)
: pathSegmantsFromSource(importSource);
const pathParts = pathSegmantsFromSource(importSource);
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, this is not the most useful way to fix this issue. I would rather you never reference the image source, and instead operate on the relative source from the root of the app. Otherwise, I don’t think you’ll ever be able to get this to actually reflect what we want to enforce

@cartogram cartogram force-pushed the fixt-strict-component-boundaries branch 4 times, most recently from f7b5337 to 6a4bacb Compare September 23, 2018 01:44
@cartogram cartogram closed this Sep 23, 2018
@cartogram cartogram reopened this Sep 23, 2018
@@ -11,51 +11,62 @@ const parserOptions = {
const errors = [
{
type: 'ImportDeclaration',
message: 'Strict component boundaries.',
message: `Do not reach into an individual component's folder for nested modules. Import from the closest shared components folder instead.`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @GoodForOneFare with regards to #122

@cartogram
Copy link
Contributor Author

cartogram commented Sep 23, 2018

Update:

  • This rule now operates only on the full relative source. There are fewer edge cases when doing it this way.
  • This meant adding a "filename" property to each test that resolves to an actual fixture file. Without this, the resolvedSource would return undefined/missing.
  • I also updated the error message to be less vague. strict-component-boundaries error message is vague #122

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Nice, working on absolute paths feels like it protects against false-positives when people do odd things with convoluted relative paths (e.g this should now consistently pass both examples).

Can we add an extra three test cases:

  • when a component imports a sibling component using a not the simplest relative path
  • when a component's subcomponents import a sibling subcomponent
  • when a subcomponent imports its parent's SCSS

The following tests should all pass:

{
  code: `import {someThing} from '../../components/Bar';`,
  parserOptions,
  filename: fixtureFile('basic-app/app/components/Foo/index.js'),
},
{
  code: `import {someThing} from '../Baz';`,
  parserOptions,
  filename: fixtureFile('basic-app/app/components/Foo/components/Bar/index.js'),
},
{
  code: `import {someThing} from '../../Foo.scss';`,
  parserOptions,
  filename: fixtureFile('basic-app/app/components/Foo/components/Bar/index.js'),
}

@cartogram cartogram force-pushed the fixt-strict-component-boundaries branch from 6a4bacb to 0a93872 Compare September 24, 2018 20:50
@cartogram
Copy link
Contributor Author

@BPScott updated with the tests you posted.

@cartogram cartogram force-pushed the fixt-strict-component-boundaries branch from a700321 to 1a4afa1 Compare September 24, 2018 21:34
@cartogram cartogram merged commit f51113f into master Sep 25, 2018
@cartogram cartogram deleted the fixt-strict-component-boundaries branch September 25, 2018 18:38
cartogram pushed a commit that referenced this pull request Oct 2, 2018
…/eslint-plugin-shopify into prefer-explicit-relative-imports

* 'prefer-explicit-relative-imports' of github.com:Shopify/eslint-plugin-shopify:
  review feedback
  prefer-explicit-local-imports
  v25.0.01
  restoring prettier parser in the typescript config
  v25.0.0
  Fixes strict-component-boundaries (#160)
  updating prettier config to enable the prettier/prettier rule

# Conflicts:
#	README.md
#	docs/rules/prefer-explicit-local-imports.md
#	index.js
#	lib/config/rules/shopify.js
cartogram pushed a commit that referenced this pull request Oct 9, 2018
…fy into add-comments

* 'add-comments' of github.com:Shopify/eslint-plugin-shopify:
  Adds `eslint-plugin-eslint-comments`
  v25.0.01
  restoring prettier parser in the typescript config
  v25.0.0
  Fixes strict-component-boundaries (#160)
  updating prettier config to enable the prettier/prettier rule
  v24.2.0
  adding graphql config to index
  updating no-vague-titles rule to fix parsing issues in getMethodName
  v24.1.1
  v24.1.0
  adding eslint-plugin-graphql as dependency and new graphql config
  updating nvmrc version to match module node engine version spec

# Conflicts:
#	CHANGELOG.md
#	lib/config/rules/eslint-comments.js
#	package.json
#	yarn.lock
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strict-component-boundaries possibly flagging incorrectly
3 participants