JsChecker: support extensionless ES6 imports

[robfig]

I want to use extensionless ES6 imports, e.g.

import { foo } from '/path/to/file';

where the module is defined in //path/to/file.js

That seems to work with closure_js_binary without issue, but JsChecker complains. Copying 3 lines from NodeModuleResolver causes the right thing to happen.

If @jart could shed any light on what she had in mind for "Unify path resolution with ModuleLoader", that might be a more robust solution, but this seems to work acceptably.

[Yannic]

Can we add a test for this?

[robfig]

Test added, but that reminded me of a lingering problem -- moduleLoad error always fires spuriously. Since moduleLoad seems to be redundant to strictDependencies, I suppressed it unconditionally. But probably for the real rules it should be made to work...

[robfig]

Hm, I see similar moduleLoad suppressions in the ES6 tests in closure/compiler/test/strict_dependency_checking/BUILD, so that makes me think that it never worked.

Perhaps the rules should just always suppress moduleLoad?

[Yannic]

Sorry for the delay!

Yeah, the moduleLoad seems to be an existing issue in rules_closure. I'm a bit hesitant to add suppresses by default, so we should fix the root cause instead. Can you create an issue for that?

Regarding this PR, I'm deferring to @alexeagle et al. to decide whether that's something rules_closure should do or not.

[robfig]

Opened an issue for the moduleLoad problem

I'm not sure I understand your other comment -- perhaps rules_closure should not support extensionless imports, or perhaps the fix instead belongs somewhere else?

Thanks for taking a look

[robfig]

@alexeagle any thoughts on getting this in? It seems pretty small / unobjectionable to me.

[Yannic]

Sorry for taking so long to come back to this!

The longer I think about this, the more opposed I get to allow this. I can't cite the relevant spec right now, but, IIRC, ES6 module imports are supposed to be paths to existing files (which is not true for extensionless imports). IMHO, this should be fixed in the tools producing this broken code.

OTOH, I also understand that extensionless imports aren't that uncommon. WDYT about allowing it only behind a flag --@io_bazel_rules_closure//closure:allow_extensionless_imports.

[robfig]

Extensionless imports are explicitly supported by Closure Compiler in NODE resolution mode, which is where I got the idea originally: https://github.com/google/closure-compiler/wiki/JS-Modules#node-resolution-mode

I don't know the history or context behind them, but after pretty much investigation, this is by far the cleanest way I've found to enable VSCode and other IDEs to understand imports and enable showing info on hover or the ability to click into imported files, for JSX. Since this is supporting a Compiler feature, required for reasonable interop, and I don't really see a downside, I would advocate for not requiring a flag.

We can set the flag in our .bazelrc or something if necessary, of course.

[Yannic]

Doesn't node resolution mode also allow importing folders (IIRC by loading index.js if it exists), making import statements potentially ambiguous, thus breaking incremental type-checking?

Let's say we have the following workspace:

/
  lib/
    BUILD
    foo.js
    bar.js
    BUILD
    foo/index.js
    foo/baz.js
  app/
    BUILD
    main.js

We declare one closure_js_library target per .js file, //lib:bar depends on //lib:foo, //libs/foo:baz depends on //lib/foo:index, and //app:main depends on //lib:bar and //lib/foo:baz.

• lib/bar.js imports lib/foo.js as /lib/foo (allowed by resolution-mode node) -> Typecheks fine with this PR.
• lib/foo/baz.js imports lib/foo/index.js as /lib/foo (allowed by resolution-mode node) -> Typecheks fine if we implement all of resolution-mode node.
• app/main.js imports lib/bar.js and lib/foo/baz.js with their full import path. However, since we have a (transitive) dependency on lib/foo.js and lib/foo/index.js, both as /lib/foo, one will get precedence over the other (IIUC, it should be lib/foo.js) and the compilation of main.js breaks even though library-level checks were fine.

[robfig]

Sorry, I missed your last response.

Relevant link: https://nodejs.org/api/modules.html#modules_all_together

I agree that your scenario is possible in theory, but it's not possible in practice because the LOAD_AS_DIRECTORY algorithm requires a package.json file, so I don't believe it's possible to trigger that case using rules_closure. Regardless, JsChecker already catches a fraction of the errors, and I think that an additional unusual case which it doesn't catch would not substantially change the developer experience.

I suggest that JsChecker supports "LOAD_AS_FILE" but not "LOAD_AS_DIRECTORY".

[gkdn]

There hasn't been any activity in this PR for very long time. Please let me know if there are any intentions to pursue this change. Otherwise I will close it. Thanks.