Skip to content

Commit

Permalink
Breaking: remove extraneous check from no-unpublish-* (fixes #71)
Browse files Browse the repository at this point in the history
  • Loading branch information
mysticatea committed Jun 2, 2017
1 parent a75c638 commit d4e6bc4
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 116 deletions.
16 changes: 3 additions & 13 deletions docs/rules/no-unpublished-import.md
Expand Up @@ -6,26 +6,16 @@ This is similar to [no-unpublished-require](no-unpublished-require.md), but this

## Rule Details

If a source code file satisfies all of the following conditions, the file is \*published*.
If a source code file satisfies all of the following conditions, the file is \*published\*.

- `"files"` field of `package.json` includes the file. (or `"files"` field of `package.json` does not exist)
- `"files"` field of `package.json` includes the file or `"files"` field of `package.json` does not exist.
- `.npmignore` does not include the file.

**This rule disallows importing the following things from the \*published\* files.**

- Unpublished files.
- Extraneous modules.
- Modules in `devDependencies`.
Then this rule warns `import` declarations in \*published\* files if the `import` declaration imports \*unpublished\* files or the packages of `devDependencies`.

> This intends to prevent "Module Not Found" error after `npm publish`.<br>
> :bulb: If you want to import `devDependencies`, please write `.npmignore` or `"files"` field of `package.json`.
**This rule disallows importing the following things from the \*unpublished\* files.**

- Extraneous modules.

> This intends to prevent "Module Not Found" error after `npm install`.
## Options

```json
Expand Down
22 changes: 6 additions & 16 deletions docs/rules/no-unpublished-require.md
@@ -1,30 +1,20 @@
# Disallow `require()`s which import unpublished files/modules (no-unpublished-require)
# Disallow `require()` expressions which import unpublished files/modules (no-unpublished-require)

If a `require()`'s target is not published, the program works in local, but will not work after published to npm.
This rule disallows `require()` of unpublished files/modules.
If a `require()` expression's target is not published, the program works in local, but will not work after published to npm.
This rule disallows `require()` expressions of unpublished files/modules.

## Rule Details

If a source code file satisfies all of the following conditions, the file is \*published*.
If a source code file satisfies all of the following conditions, the file is \*published\*.

- `"files"` field of `package.json` includes the file. (or `"files"` field of `package.json` does not exist)
- `"files"` field of `package.json` includes the file or `"files"` field of `package.json` does not exist.
- `.npmignore` does not include the file.

**This rule disallows importing the following things from the \*published\* files.**

- Unpublished files.
- Extraneous modules.
- Modules in `devDependencies`.
Then this rule warns `require()` expressions in \*published\* files if the `require()` expression imports \*unpublished\* files or the packages of `devDependencies`.

> This intends to prevent "Module Not Found" error after `npm publish`.<br>
> :bulb: If you want to import `devDependencies`, please write `.npmignore` or `"files"` field of `package.json`.
**This rule disallows importing the following things from the \*unpublished\* files.**

- Extraneous modules.

> This intends to prevent "Module Not Found" error after `npm install`.
## Options

```json
Expand Down
8 changes: 5 additions & 3 deletions lib/util/check-extraneous.js
Expand Up @@ -58,11 +58,13 @@ module.exports = function checkForExtraeous(context, filePath, targets) {
const allowed = new Set(getAllowModules(context))
const convertPath = getConvertPath(context)
const rootPath = path.dirname(packageInfo.filePath)
const toRelative = function(fullPath) { // eslint-disable-line func-style
const retv = path.relative(rootPath, fullPath).replace(/\\/g, "/")
return convertPath(retv)
}
const convertedPath = path.resolve(
rootPath,
convertPath(
path.relative(rootPath, path.resolve(filePath))
)
toRelative(path.resolve(filePath))
)
const basedir = path.dirname(convertedPath)
const dependencies = new Set(
Expand Down
44 changes: 14 additions & 30 deletions lib/util/check-publish.js
Expand Up @@ -36,48 +36,32 @@ module.exports = function checkForPublish(context, filePath, targets) {
return
}

const allowed = getAllowModules(context)
const allowed = new Set(getAllowModules(context))
const convertPath = getConvertPath(context)
const basedir = path.dirname(packageInfo.filePath)
const toRelative = function(fullPath) { // eslint-disable-line func-style
const retv = path.relative(basedir, fullPath).replace(/\\/g, "/")
return convertPath(retv)
}
const npmignore = getNpmignore(filePath)
const dependencies = Object.assign(
Object.create(null),
packageInfo.peerDependencies || {},
packageInfo.dependencies || {}
)
const devDependencies = Object.assign(
Object.create(null),
packageInfo.devDependencies || {}
const devDependencies = new Set(
Object.keys(packageInfo.devDependencies || {})
)

if (npmignore.match(toRelative(filePath))) {
// This file is private, so this can import private files.
for (const target of targets) {
if (target.moduleName &&
!dependencies[target.moduleName] &&
!devDependencies[target.moduleName] &&
allowed.indexOf(target.moduleName) === -1
) {
context.report({
node: target.node,
loc: target.node.loc,
message: "\"{{name}}\" is not published.",
data: {name: target.moduleName},
})
}
}
}
else {
if (!npmignore.match(toRelative(filePath))) {
// This file is published, so this cannot import private files.
for (const target of targets) {
if (target.moduleName ?
(!dependencies[target.moduleName] && allowed.indexOf(target.moduleName) === -1) :
const isPrivateFile = (
target.moduleName == null &&
npmignore.match(toRelative(target.filePath))
) {
)
const isDevPackage = (
target.moduleName != null &&
devDependencies.has(target.moduleName) &&
!allowed.has(target.moduleName)
)

if (isPrivateFile || isDevPackage) {
context.report({
node: target.node,
loc: target.node.loc,
Expand Down
33 changes: 8 additions & 25 deletions tests/lib/rules/no-unpublished-import.js
Expand Up @@ -130,16 +130,6 @@ ruleTester.run("no-unpublished-import", rule, {
},
],
invalid: [
{
filename: fixture("1/test.js"),
code: "import noDeps from 'no-deps';",
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("1/test.js"),
code: "import c from 'no-deps/a/b/c';",
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("2/test.js"),
code: "import ignore1 from './ignore1.js';",
Expand All @@ -150,16 +140,6 @@ ruleTester.run("no-unpublished-import", rule, {
code: "import ignore1 from './ignore1';",
errors: ["\"./ignore1\" is not published."],
},
{
filename: fixture("2/ignore1.js"),
code: "import noDeps from 'no-deps';",
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("3/test.js"),
code: "import noDeps from 'no-deps';",
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("3/pub/test.js"),
code: "import bbb from 'bbb';",
Expand Down Expand Up @@ -193,11 +173,6 @@ ruleTester.run("no-unpublished-import", rule, {
},

// Should work fine if the filename is relative.
{
filename: "tests/fixtures/no-unpublished/2/test.js",
code: "import noDeps from 'no-deps';",
errors: ["\"no-deps\" is not published."],
},
{
filename: "tests/fixtures/no-unpublished/2/test.js",
code: "import ignore1 from './ignore1';",
Expand Down Expand Up @@ -255,5 +230,13 @@ ruleTester.run("no-unpublished-import", rule, {
tryExtensions: [".js", ".jsx", ".json"],
}],
},

// outside of the package.
{
filename: fixture("1/test.js"),
code: "import a from '../2/a.js';",
env: {node: true},
errors: ["\"../2/a.js\" is not published."],
},
],
})
36 changes: 7 additions & 29 deletions tests/lib/rules/no-unpublished-require.js
Expand Up @@ -244,18 +244,6 @@ ruleTester.run("no-unpublished-require", rule, {
},
],
invalid: [
{
filename: fixture("1/test.js"),
code: "require('no-deps');",
env: {node: true},
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("1/test.js"),
code: "require('no-deps/a/b/c');",
env: {node: true},
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("2/test.js"),
code: "require('./ignore1.js');",
Expand All @@ -268,18 +256,6 @@ ruleTester.run("no-unpublished-require", rule, {
env: {node: true},
errors: ["\"./ignore1\" is not published."],
},
{
filename: fixture("2/ignore1.js"),
code: "require('no-deps');",
env: {node: true},
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("3/test.js"),
code: "require('no-deps');",
env: {node: true},
errors: ["\"no-deps\" is not published."],
},
{
filename: fixture("3/pub/test.js"),
code: "require('bbb');",
Expand Down Expand Up @@ -367,15 +343,17 @@ ruleTester.run("no-unpublished-require", rule, {
// Should work fine if the filename is relative.
{
filename: "tests/fixtures/no-unpublished/2/test.js",
code: "require('no-deps');",
errors: ["\"no-deps\" is not published."],
code: "require('./ignore1');",
errors: ["\"./ignore1\" is not published."],
env: {node: true},
},

// outside of the package.
{
filename: "tests/fixtures/no-unpublished/2/test.js",
code: "require('./ignore1');",
errors: ["\"./ignore1\" is not published."],
filename: fixture("1/test.js"),
code: "require('../2/a.js');",
env: {node: true},
errors: ["\"../2/a.js\" is not published."],
},
],
})

0 comments on commit d4e6bc4

Please sign in to comment.