New rules regarding namespaces used as values

[dead-claudia]

The following patterns are considered not warnings:

import * as foo from "./foo";

foo.bar();
const baz = foo.baz;
const {property} = foo;

The following patterns are considered warnings:

import * as foo from "./foo";

foo();
const [value] = foo;
func(foo);
func(...foo);
const alias = foo;
const {child} = foo; // foo.child is an exported namespace

---

Basically, warn if a namespace is used in a way that cannot be validated (aliased, called, destructured as array, passed as argument, etc.). There's no overlap with namespace.allowComputed in the above, but that option could be merged into this one, or these merged into another namespace option.

[benmosher]

I like it.

Probably makes sense as a separate rule (vs. an option on namespace), as it does not require the namespace (and imported file) to be resolved to execute properly.

I don't understand this part of the warnings, though:

const {child} = foo; // foo.child is an exported namespace

Are you saying destructuring to a deep namespace is not allowed?

If so, if the import was resolved, it would actually know whether child was a namespace or something else, and thus could allow the destructure but warn on passing child to functions as an arg, calling it as a function, etc.

I think, to that end, that even if this rule did use the resolver infrastructure for that, it makes sense as a separate rule. namespace's responsibility is to ensure that properties exist.

[jfmengels]

👍 for a different rule.

The idea is good, but I agree that some of the failing examples should be fine:

func(foo);
func(...foo);
const {child} = foo;

This use seems a bit odd, but not wrong though I think (or please enlighten me).

[dead-claudia]

@jfmengels

Regarding func(foo) and const {child} = foo (if foo.child is a namespace), I don't mean it's wrong per se, but Rollup has to generate a frozen object in both of those cases. If you prohibit those via linter errors, Rollup can generate much smaller, more minifier-friendly code. That's more of an opinion than an objective error.

I'll note that import * as ns from "./ns"; func(...ns) will always throw a type error at runtime, though, because ns can't implement Symbol.iterator by virtue of it being a frozen map of identifier->getter pairs (valid identifier to getter pairs).

[dead-claudia]

@benmosher

Probably makes sense as a separate rule (vs. an option on namespace), as it does not require the namespace (and imported file) to be resolved to execute properly.

I agree it should be its own rule, I just proposed the option idea in case that was too much.

Are you saying destructuring to a deep namespace is not allowed?

Correct.

I think, to that end, that even if this rule did use the resolver infrastructure for that, it makes sense as a separate rule. namespace's responsibility is to ensure that properties exist.

I do feel this might actually be best as a set of related rules:

• Error: no-namespace-call → ns(...args) is invalid.
• Error: no-namespace-destructure-array → const [child] = ns is invalid
• Warning: no-namespace-destructure-alias → const {child} = ns is invalid
• Warning: no-namespace-alias → const alias = ns is invalid
• Warning: no-namespace-as-value → func(ns), [foo], etc. is invalid
• Error: no-namespace-as-spread → func(...ns) and [...ns] is invalid

Errors are basically static errors that can be known with minimal type context to generate a runtime error (and any sane config will reject them). Warnings are a matter of opinion, but they can increase file size of Rollup bundles if you're not careful (they require a frozen object to be created).

[benmosher]

I like where that's heading, a rule cluster makes sense. Though I think maybe the existing namespace rule (or a single new no-namespace-misbehaviorrule) should absorb the cases that are well-known to be runtime errors (e.g. calling as function, spreading, array destructuring).

The ones that remain:

• Warning: no-namespace-destructure-alias → const {child} = ns is invalid
• Warning: no-namespace-alias → const alias = ns is invalid
• Warning: no-namespace-as-value → func(ns), [foo], etc. is invalid

make sense separately as style-guide rules (in service of ideal Rollup bundling).

[dead-claudia]

I like that idea, although I'll bikeshed the name of the catch-all known
error rule (I'd prefer something like no-invalid-namespace-use, to be a
little more descriptive of what it's for).

On Tue, Aug 30, 2016, 11:33 Ben Mosher notifications@github.com wrote:

I like where that's heading, a rule cluster makes sense. Though I think
maybe the existing namespace rule (or a single new
no-namespace-misbehaviorrule) should absorb the cases that are well-known
to be runtime errors (e.g. calling as function, spreading, array
destructuring).

The ones that remain:

• Warning: no-namespace-destructure-alias → const {child} = ns is
  invalid
• Warning: no-namespace-alias → const alias = ns is invalid
• Warning: no-namespace-as-value → func(ns), [foo], etc. is invalid

make sense separately as style-guide rules (in service of ideal Rollup
bundling).

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#532 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AERrBKXH2mwlplqCaYHgWNOCcLbpwBq_ks5qlE07gaJpZM4JvMLq
.

[benmosher]

👍

[dead-claudia]

I'll admit I'm not that familiar with writing ESLint rules, though. So I'm
probably not the best equipped to write a PR for it.

On Thu, Sep 1, 2016, 04:50 Ben Mosher notifications@github.com wrote:

👍

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#532 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AERrBDebgD8LFhCDLJmQHzKVYIubj7bfks5qlpHbgaJpZM4JvMLq
.