Skip to content

Commit

Permalink
#384: unambiguous docs + added to recommended config as a warning
Browse files Browse the repository at this point in the history
  • Loading branch information
benmosher committed Jul 20, 2016
1 parent a91e93a commit 9844c62
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 0 deletions.
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a

**Helpful warnings:**


* Report any invalid exports, i.e. re-export of the same name ([`export`])
* Report use of exported name as identifier of default export ([`no-named-as-default`])
* Report use of exported name as property of default export ([`no-named-as-default-member`])
Expand All @@ -43,10 +44,12 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a

**Module systems:**

* Report potentially ambiguous parse goal (`script` vs. `module`) ([`unambiguous`])
* Report CommonJS `require` calls and `module.exports` or `exports.*`. ([`no-commonjs`])
* Report AMD `require` and `define` calls. ([`no-amd`])
* No Node.js builtin modules. ([`no-nodejs-modules`])

[`unambiguous`]: ./docs/rules/unambiguous.md
[`no-commonjs`]: ./docs/rules/no-commonjs.md
[`no-amd`]: ./docs/rules/no-amd.md
[`no-nodejs-modules`]: ./docs/rules/no-nodejs-modules.md
Expand Down
1 change: 1 addition & 0 deletions config/recommended.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module.exports = {
'import/no-named-as-default': 'warn',
'import/no-named-as-default-member': 'warn',
'import/no-duplicates': 'warn',
'import/unambiguous': 'warn',
},

// need all these for parsing dependencies (even if _your_ code doesn't need
Expand Down
54 changes: 54 additions & 0 deletions docs/rules/unambiguous.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# unambiguous

Warn if a `module` could be mistakely parsed as a `script` by a consumer leveraging
[Unambiguous JavaScript Grammar] to determine correct parsing goal.

Will respect the [`parserOptions.sourceType`] from ESLint config, i.e. files parsed
as `script` per that setting will not be reported.

This plugin uses [Unambiguous JavaScript Grammar] internally to decide whether
dependencies should be parsed as modules and searched for exports matching the
`import`ed names, so it may be beneficial to keep this rule on even if your application
will run in an explicit `module`-only environment.

## Rule Details

For files parsed as `module` by ESLint, the following are valid:

```js
import 'foo'
function x() { return 42 }
```

```js
export function x() { return 42 }
```

```js
(function x() { return 42 })()
export {} // simple way to mark side-effects-only file as 'module' without any imports/exports
```

...whereas the following file would be reported:
```js
(function x() { return 42 })()
```

## When Not To Use It

If your application environment will always know via [some other means](https://github.com/nodejs/node-eps/issues/13)

This comment has been minimized.

Copy link
@jdalton

jdalton Jul 20, 2016

The link to issue/13 is a bit confusing. It could instead link to the MDN browser module definition which is signaled by a type="module" attribute – https://mdn.io/script#attr-type

how to parse, regardless of syntax, you may not need this rule.

Remember, though, that this plugin uses this strategy internally, so if you were
to `import` from a module with no `import`s or `export`s, this plugin would not
report it as it would not be clear whether it should be considered a `script` or
a `module`.

This comment has been minimized.

Copy link
@jdalton

jdalton Jul 20, 2016

This last bit is unclear to me. What's the warning about? A module with no import or export which then import would have an import?

This comment has been minimized.

Copy link
@benmosher

benmosher Jul 20, 2016

Author Member
// foo.js
import { x } from './bar.js'

// bar.js
function x() { return 42 }

in this case, the import in foo.js would not be flagged by any of the static analysis rules because it is disambiguating with CJS under the covers using this same strategy. So the fact that x is not exported, while likely potentially a mistake, may go unnoticed unless the unambiguous rule is enabled.

This comment has been minimized.

Copy link
@jdalton

jdalton Jul 20, 2016

I think that's covered by the valid rules examples. This seems super wordy for something that's covered by the definition of unambiguous grammar (must have at least an import or export declaration).

This comment has been minimized.

Copy link
@benmosher

benmosher Jul 20, 2016

Author Member

Fair enough.

I think the thing that is not obvious is that this plugin itself is dependent on your modules being unambiguously modules, even if the ultimate execution environment is not. That is what I was trying to separately emphasize here.

I'm not surprised that I was not successful, heh. 😅 It may not be worth noting, anyway.

This comment has been minimized.

Copy link
@jdalton

jdalton Jul 20, 2016

Ya, I'm still confused.

Ah, I think I have it now. This could be cleared up in the first mention.

This plugin uses Unambiguous JavaScript Grammar internally to decide whether dependencies should be parsed as modules and searched for exports matching the imported names

It could be tweaked to state:

When a parserOptions.sourceType is not set, this plugin uses Unambiguous JavaScript Grammar internally to decide whether dependencies should be parsed as modules and searched for exports matching the imported names

This comment has been minimized.

Copy link
@benmosher

benmosher Jul 21, 2016

Author Member

It could, but that would be inaccurate.

Inside a rule, I only have access to the parse goal for the file currently being linted.

For rules like named, I have to parse the files that the linted file imports looking for exports.

If I don't find any (possibly because the module is CJS), I don't report anything because in that case, I have to assume that the linted file is leaning on Babel or whatever to make it work at runtime, and I can't analyze the target dependency anyway.

If I find some exports (or now, given this, imports count too), but not the ones mentioned by name in the ImportDeclaration, that's reported as an error.

Unfortunately, when I do this, I don't have access to parser options for the dependencies. For node_modules, I couldn't even expect to have them. (I think I may see if I can get the actual ESLint config for internal modules, though... haven't tried before.)

That's the internal disambiguation to which I am referring.


## Further Reading

- [Unambiguous JavaScript Grammar]
- [`parserOptions.sourceType`]
- [node-eps#13](https://github.com/nodejs/node-eps/issues/13)

[`parserOptions.sourceType`]: http://eslint.org/docs/user-guide/configuring#specifying-parser-options
[Unambiguous JavaScript Grammar]: https://github.com/bmeck/UnambiguousJavaScriptGrammar

This comment has been minimized.

Copy link
@jdalton

jdalton Jul 20, 2016

0 comments on commit 9844c62

Please sign in to comment.