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

[strict-component-boundaries] Exclude node_modues #140

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

cartogram
Copy link
Contributor

@cartogram cartogram commented Aug 3, 2018

@lemonmade took a stab at this one, let me know what you think of the approach and how I would go about testing this.

The issue I am having is that in order for the inNodeModules function to work, the package needs to actually be in node_modules and to test that I need to have a module installed that breaks the strict-component-imports rule, which I don't.

For example, I would need to have import {Subscription} from 'apollo-client/util/Observable'; installed to this project.

cc/ @TheMallen

@@ -25,6 +25,14 @@ ruleTester.run('strict-component-boundaries', rule, {
code: `import {someThing} from '../OtherComponent';`,
parserOptions,
},
{
code: `import fs from 'fs';`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these 2 added tests are not right, they should have something that would otherwise trigger this rule, but because they are in node modules (or a core package) they would be valid.

return true;
}

if (!resolvedSource) {
Copy link
Member

Choose a reason for hiding this comment

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

What do the two checks above do?

Copy link
Contributor Author

@cartogram cartogram Aug 4, 2018

Choose a reason for hiding this comment

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

resolvedSource === null if it is a core library
resolvedSource === undefined if it is not installed

finally if we get a string, its the full path to the module.

return false;
}

return resolvedSource.match(/node_modules/);
Copy link
Member

Choose a reason for hiding this comment

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

Usually better to use regex.test since it returns a real Boolean

{
code: `import eslint from 'eslint';`,
parserOptions,
},
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to test that this actually works for a path that is in node modules and that does not adhere to our rules (something/MyComponent)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to actually install something/MyComponent, otherwise I don't think this will work.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙇

@cartogram cartogram force-pushed the fix-only-check-local-modules branch from c6b355d to 17e5718 Compare August 4, 2018 10:25
@cartogram
Copy link
Contributor Author

@lemonmade cleaned things up and added the node_modules test.

@cartogram cartogram force-pushed the fix-only-check-local-modules branch from 17e5718 to 287915b Compare August 4, 2018 10:38
@cartogram cartogram force-pushed the fix-only-check-local-modules branch from 287915b to f5e3458 Compare August 4, 2018 10:46
@cartogram cartogram requested a review from TzviPM August 14, 2018 16:52
@@ -32,10 +39,22 @@ module.exports = {
},
};

function isCoreModule(resolvedSource) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a CoreModule ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A module that comes with node core

{
code: `import {getDisplayName} from '@shopify/react-utilities/components';`,
parserOptions,
filename: fixtureFile('basic-app/app/sections/MySection/MySection.js'),
Copy link
Contributor

Choose a reason for hiding this comment

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

What it does fixtureFile ?

@cartogram cartogram merged commit 06687ba into master Aug 20, 2018
@cartogram cartogram deleted the fix-only-check-local-modules branch August 25, 2018 19:58
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.

3 participants