-
Notifications
You must be signed in to change notification settings - Fork 220
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
Fix no-extraneous-dependencies: Take 2 #1064
Conversation
70b4b18
to
d1405c7
Compare
2aee247
to
af00114
Compare
@@ -24,6 +24,7 @@ | |||
"dependencies": { | |||
"@types/koa-bodyparser": "*", | |||
"@types/koa-compose": "*", | |||
"@types/koa": "^2.0.50", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library uses the Context
from koa
and would require the consumer to have koa
installed in their project. There were a number of packages that met this criteria, and I use the following method to resolve them: 1) Add the packages the host is required to have in the peerDependencies
with a pretty loose version range, and 2) add the types packages used to the dependencies.
Why? I added the peerDependency because it is needed (yet unused) in this library. You can't really use a koa
middleware as intended without koa
, but you also don't need koa
to write them. They are, more or less, just functions. I added a loose version range to reduce the chance 2 versions of the package will be installed, in this case koa
, as a result of this library.
I added the types in dependencies because there is a middleware, persistedOperationAssociationMiddleware
, that uses the Context
type in one of its arguments and that function is being exported. Even though the use of the type is tangental, we are not exporting the type itself or expecting consumers will use it, they will use the function that uses the type. For this reason, it needed to be in dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good rationale. Is this version range too restrictive, though?
af00114
to
2cbc396
Compare
packages/react-i18n/src/i18n.ts
Outdated
@@ -1,3 +1,6 @@ | |||
import {memoize as memoizeFn} from '@shopify/function-enhancers'; | |||
import {memoize} from '@shopify/decorators'; | |||
import {languageFromLocale, regionFromLocale} from '@shopify/i18n'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the resolver changes and re-classification of these libraries (from "internal" to "external"), they are being rearranged as a result of import/order
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still not clear to me why they should have changed position relative to @shopify/dates
?
@@ -24,6 +24,7 @@ | |||
"dependencies": { | |||
"@types/koa-bodyparser": "*", | |||
"@types/koa-compose": "*", | |||
"@types/koa": "^2.0.50", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good rationale. Is this version range too restrictive, though?
packages/react-i18n/package.json
Outdated
@@ -34,6 +34,8 @@ | |||
"intl-pluralrules": "^0.2.1" | |||
}, | |||
"dependencies": { | |||
"@babel/template": "^7.6.0", | |||
"@babel/traverse": "^7.6.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure if these are strictly dependencies. Aren't they just used for types? Babel should generally be a peer/ optional dependency IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the FYI and the thorough explanation. It makes sense to me 👍. I just had one minor comment/nitpick.
packages/jest-koa-mocks/src/create-mock-context/create-mock-context.ts
Outdated
Show resolved
Hide resolved
fa3a254
to
f248608
Compare
I wonder if there's some upstream PR we can make to https://github.com/alexgorbatchev/eslint-import-resolver-typescript to allow support for our monorepo use case |
82a2d57
to
11a855a
Compare
@@ -0,0 +1,13 @@ | |||
// We need to specify the import/resolver setting in an extendable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this works - rules later in the config overrode earlier ones - it's how you can enable a rule in a config, then disable it in your local eslint config. I would have thought settings would work in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the import plugin iterates over the setting object, which gets merged together by ESLint's config. However, it merges in the order of plugin addition, and then iterates over the object (iteration happens in insertion order), so resolvers added by earlier plugins are actually handled first.
4202a6d
to
649cf4c
Compare
328103c
to
36e174c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, still a few more questions 👍
packages/react-server/package.json
Outdated
"koa-mount": "^4.0.0" | ||
"koa-mount": "^4.0.0", | ||
"fs-extra": "^8.1.0", | ||
"isomorphic-fetch": "2.2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually used for it directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't no. It is just imported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might make more sense as a peer dependency.
packages/react-server/package.json
Outdated
"chalk": "^2.4.2", | ||
"koa": "^2.5.0", | ||
"koa-compose": "^4.1.0", | ||
"koa-mount": "^4.0.0" | ||
"koa-mount": "^4.0.0", | ||
"fs-extra": "^8.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used for checking if there is a Gemfile, when getting the manifest file. See pathExistsSync
below.
function getManifestPath(root: string) {
const gemFileExists = pathExistsSync(join(root, 'Gemfile'));
if (!gemFileExists) {
return;
}
// eslint-disable-next-line no-process-env
return process.env.NODE_ENV === 'development'
? `tmp/sewing-kit/sewing-kit-manifest.json`
: `public/bundles/sewing-kit-manifest.json`;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's an alias for the built in fs.existsSync.
https://github.com/jprichardson/node-fs-extra/blob/master/lib/path-exists/index.js#L11
433a2db
to
271d749
Compare
271d749
to
ea23718
Compare
@@ -27,14 +27,15 @@ | |||
"@shopify/network": "^1.4.2", | |||
"@shopify/performance": "^1.2.0", | |||
"@shopify/statsd": "^1.1.0", | |||
"@types/koa": "^2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird spacing
"@types/koa-bodyparser": "*", | ||
"@types/koa-compose": "*", | ||
"change-case": "^3.1.0", | ||
"koa-bodyparser": ">=4.0.0 <5.0.0", | ||
"koa-compose": ">=3.0.0 <4.0.0" | ||
}, | ||
"peer-dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops
packages/react-graphql/package.json
Outdated
"apollo-cache-inmemory": "^1.6.2", | ||
"apollo-client": "^2.6.0", | ||
"apollo-link": "^1.2.13", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want a much less restrictive version here
packages/react-i18n/package.json
Outdated
"intl-pluralrules": "^0.2.1" | ||
}, | ||
"dependencies": { | ||
"@babel/types": ">=7.0.0", | ||
"@types/babel__template": "^7.0.0", | ||
"@types/babel__traverse": "^7.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had a bit of a change of heart on this, while technically they do make sense as dependencies, in practice they probably don't because you never consume the Babel dependency directly (only Babel consumes it). So, IMO, dev dependencies
packages/react-i18n/src/i18n.ts
Outdated
@@ -1,3 +1,6 @@ | |||
import {memoize as memoizeFn} from '@shopify/function-enhancers'; | |||
import {memoize} from '@shopify/decorators'; | |||
import {languageFromLocale, regionFromLocale} from '@shopify/i18n'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still not clear to me why they should have changed position relative to @shopify/dates
?
packages/web-worker/package.json
Outdated
"webpack-virtual-modules": "^0.1.12" | ||
"webpack-virtual-modules": "^0.1.12", | ||
"koa-mount": "^3.0.0", | ||
"@types/koa-mount": "^3.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither of the above are dependencies, both can be in devDependencies
Description
Fixes #1001
Follow up from #1026
This PR is split into 2 commits. The first contains only the changes required to make the
import/no-extraneous-dependencies
rule work for this repo, the second is all the lint fixes required to pass CI.The fix here was to use the
import/resolver
setting to override the typescript resolver fromplugin:shopify/typescript
. The typescript resolver was problematic because it resolves modules to their source location, causing the rule think of them as "internal" (not extraneous) modules. Creating a new configuration and extending it before extending theplugin:shopify/typescript
tells the rule to use the node resolver, which looks for these packages innode_modules
instead and properly sees them as "external" to the eslint rule.The downside to having this rule enabled is that it (correctly) enforces all our
@shopify/*
dependencies to be listed when they are used and thus increases the dependency web and number of "patch" dependency bumps needed (releasing a new version of dependencyx
causes all related dependencies that usex
to need a new version). The largest offender of this was@shopify/react-testing
, which is used in all of the@shopify/react-*
dependencies.The fix for this was to add an override for test files that disables this rule. We don't care if
@shopify/react-testing
is listed or not since its really just needed for development and will never cause a missing dependency error when installed in a consuming package, which is the only thing we really care about here.