Skip to content

ES6 module support for "type-only import" like goog.requireType or TS import type #3041

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

Open
jplaisted opened this issue Aug 2, 2018 · 12 comments
Labels
feat Feature Request internal-issue-created An internal Google issue has been created to track this GitHub issue Modules triage-done Has been reviewed by someone on triage rotation.

Comments

@jplaisted
Copy link
Contributor

The type system currently has no way of referencing types in ES6 modules without importing them. We need some sort of weak reference / import type for ES6 modules.

Bike shedding ideas dump:

  1. Allow paths in type names. Use : as a delimiter for path:type.
/** @param {./foo.js:Type.Nested} n */
function foo(n) {}
  1. Some commented out import syntax that brings the names into the type scope.
// @import {Type} from './foo.js';

/** @param {Type.Nested} n */
function foo(n) {}
  1. Since the option above might be difficult with the current way we handle JsDoc (needs to be attached to something) and because Type doesn't really exist (it isn't a variable), we could modify the above option to act more like how we handle typedefs, i.e. annotate a variable declaration.
/** @import {Type} from './foo.js'; */
let Type;

/** @param {Type.Nested} n */
function foo(n) {}
@jplaisted jplaisted changed the title References types from ES6 modules without importing Reference types from ES6 modules without importing Aug 2, 2018
@evmar
Copy link
Contributor

evmar commented Aug 14, 2018

#1 resembles the analogous TS feature, I think. I've never used it myself but I think it was meant to look like dynamic imports, under the reasoning that imports in type position don't have runtime implications.

var x: import('./y).Type; 

https://davidea.st/articles/typescript-2-9-import-types

@jplaisted
Copy link
Contributor Author

jplaisted commented Aug 14, 2018

Ah, thanks for the info. Yeah basing this off dynamic import syntax may make a bit more sense, if we can do it.

/** @param {import('./foo.js').Type.Nested} n */
function foo(n) {}

/** @typedef {import('other/resolution').Type} */
let TypeFromOtherModule;

+ @tjgq

lauraharker pushed a commit that referenced this issue Aug 14, 2018
…ses dot (.) as a delimiter from path to type, meaning you can't use this if your module resolution isn't node (since you'd need to include the .js extension). It also only works within ES6 modules, which isn't great.

#3041 is open to implement something more robust.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=208718943
@ChadKillingsworth
Copy link
Collaborator

This isn't restricted to ES6 modules. CommonJS need the same thing.

@jplaisted
Copy link
Contributor Author

@ChadKillingsworth Do we have any documentation as to how import-ing a CommonJS module works in the closure compiler? Specifically with default CommonJS exports. I'm wondering what /** import('some-commonjs-module') */ would mean. Maybe it makes more sense to have both import() and require()? I personally disagree with trying to use ES6 import syntax for non-ES6 modules.

@ChadKillingsworth
Copy link
Collaborator

@jplaisted It's a bit complicated, but also well defined. I don't have a problem using import('module') in an annotation to get a hold of the types in a CommonJS module. I'd rather not complicate this project further and treat ES6 as the go-forward strategy.

Since typescript already does this, there is precedent.

@jplaisted
Copy link
Contributor Author

but also well defined

Defined where? I am asking for link to documentation. Doesn't have to be for the compiler specifically if the compiler just followed something else.

@ChadKillingsworth
Copy link
Collaborator

Ahh now that's the million dollar question. The "official" documentation is buried in a github issue on Node's repo somewhere and not at all clear.

Here's an overly simplistic view: https://webpack.js.org/migrate/4/#import-and-commonjs

In a nutshell, CommonJS modules are considered to have a single export property which is the default property.

@jplaisted
Copy link
Contributor Author

From what I understand Node has yet to decide whether to support the interop, and how. But I'm not an expert, that's just what I've heard.

If there's no documentation, and if we're going off Node which afaik doesn't have an officially approved propsal, then that seems to be not well defined. Plus, as far as I can tell by just messing around, TypeScript has different meaning for import() than Webpack for a CommonJS module.

That aside, I think I'll just fix this for ES6 modules and leave you to fix it for CommonJS. I don't think it will be too difficult. My plan right now is to have a new pass that runs before module rewriting that rewrites these import comments to the global symbols the modules will be rewritten to. This pass will have access to what type of module is being imported (CJS or ES6 or Closure, etc), so it will be up to you to just add an else if (isCommonJs) block.

Sound good to you?

Note: obviously in a ideal world the type checker would just understand these annotations. Problem is the type checker doesn't understand modules of any kind at all right now. Ideally this new pass goes away once the type checker runs before module rewriting.

@ChadKillingsworth
Copy link
Collaborator

Sound good to you?

Sure does.

One more thing though - we'll need to prevent these files from being dropped from the compilation early.

@erwinmombay
Copy link

erwinmombay commented Apr 3, 2019

Hey all, is there any progress on this? This is making it impossible for us to upgrade closure compiler without reverting acea045 (we actually had to)

would it be an unreasonable request to revert acea045 until #3041 is implemented.

We cannot simply do imports of the Type as the solution, since importing a module could have side effects. This would also break circular references for us (which I assume we do in our large codebase)

@jplaisted
Copy link
Contributor Author

I haven't had the time to look at this in awhile. I can bring it up at our prioritization meetings and see if we can schedule it.

Personally, I do feel it may be unreasonable to revert that PR. What we had wasn't really robust / didn't really work for most people. But you have my apologies, I probably should've checked to see who (if anyone) was using it before deleting it.

@brad4d brad4d added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Apr 4, 2019
@brad4d
Copy link
Contributor

brad4d commented Apr 4, 2019

Internal issue created http://b/129956426

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Feature Request internal-issue-created An internal Google issue has been created to track this GitHub issue Modules triage-done Has been reviewed by someone on triage rotation.
Projects
None yet
Development

No branches or pull requests

6 participants