Skip to content
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

Don't forward declare if googmodule is false #758

Closed
wants to merge 4 commits into from

Conversation

dreamofabear
Copy link

Don't emit goog.forwardDeclare if goog.module conversion is disabled.

@dreamofabear
Copy link
Author

Right, so the problem is that googmodule is a property on Es5ProcessorHost not AnnotatorHost. One way to fix it is to also add a googmodule property to AnnotatorHost. Let me know if you guys would prefer another solution.

@mprobst
Copy link
Contributor

mprobst commented Mar 7, 2018

If we don't do goog.module conversion, we'll have ES6 imports in the output. However for pure type only TypeScript-level imports, there won't be an emitted ES6 import in the output.

Without goog.forwardDeclare, how will we be able to reference types in that scenario?

@dreamofabear
Copy link
Author

However for pure type only TypeScript-level imports, there won't be an emitted ES6 import in the output.

Sorry I'm still learning TS so I don't quite follow -- why not? Which TS type declaration can't be represented either by a CC analog or Any?

@mprobst
Copy link
Contributor

mprobst commented Mar 8, 2018

Imagine you have this TS code:

import {X} from 'x';
import {Y} from 'y';
function doSomethingWith(x: X) { return new Y(); };

TypeScript will emit something like:

import {Y} from 'y';
function doSomethingWith(x) { return new Y(); };

Note how the import for X is gone. That's because it's only used for types. To retain those semantics, tsickle emits goog.forwardDeclare for Closure, so it doesn't force a load, but can still reference the type in type annotations.

Now if you change it to not emit forward declares, the code would look something like:

import {Y} from 'y';
/**
 * @param {??? what goes here ???} x
 * @return {Y}
 */
function doSomethingWith(x) { return new Y(); };

And that's the problem that I don't know how to solve :-)

@dreamofabear
Copy link
Author

Thanks for the explanation!

So IIUC, tsickle isn't able to run the annotate transform because the type-only file has disappeared. How about the following?

  1. Short-term: If googmodule is falsy, then output a warning and replace the declarations from the missing type-only file with *.
  2. Medium-term: Something hacky e.g. inject a non-type reference into type-only files to workaround this TS compiler behavior.
  3. Long-term: I'll follow up with TS folks to see if we can add this to the compiler API.

I think status quo is not great since CC fully supports ES6 and there's no way to completely opt-out of goog.module. WDYT?

@mprobst
Copy link
Contributor

mprobst commented Mar 9, 2018

@choumx I think the broken/missing piece here is that Closure does not have an equivalent of goog.forwardDeclare for ES6 modules. I don't think there's a change needed in TS compiler, IMHO it's doing the right thing here.

(1) would be possible, but currently the code just always uses the forward declared type when referencing types, so that'd complicate the code quite a bit.

Can you explain what actual problem you're trying to fix here? Maybe there's an easier way?

@dreamofabear
Copy link
Author

I think the broken/missing piece here is that Closure does not have an equivalent of goog.forwardDeclare for ES6 modules.

I think I'm still missing something. Why can't ES6 import replace goog.forwardDeclare?

In your example, let's imagine file x only contain a single TS interface:

// x.ts
export interface X {
  foo: string;
}

Expected output:

// x.js
/** @record */ 
export function X() {}
/** @type {string} */
X.prototype.foo;
// main.js
import {X} from 'x';
import {Y} from 'y';
/**
 * @param {X} x
 * @return {Y}
 */
function doSomethingWith(x) { return new Y(); };

Actual output:

// main.js
const tsickle_forward_declare_1 = goog.forwardDeclare('path/to/x');
goog.require('path/to/x'); // force type-only module to be loaded
import {Y} from 'y';
/**
 * @param {X} x
 * @return {Y}
 */
function doSomethingWith(x) { return new Y(); };

@dreamofabear
Copy link
Author

dreamofabear commented Mar 9, 2018

Can you explain what actual problem you're trying to fix here? Maybe there's an easier way?

The problem is our project's current build process doesn't play nicely with Closure modules. I could try fixing that instead, though I still don't quite understand why the "expected output" above is not feasible.

To retain those semantics, tsickle emits goog.forwardDeclare for Closure, so it doesn't force a load, but can still reference the type in type annotations.

I saw this explanation in tsickle.js which sounds similar. Does ES6 import also exhibit the "load" behavior as goog.require? I'm unfamiliar since that we apparently haven't encountered this problem on our project using pure ES6 with CC.

Any clarification would be appreciated!

@mprobst
Copy link
Contributor

mprobst commented Mar 12, 2018

The problem is that main.js will not contain import {X} from 'x';, because TypeScript does not emit runtime imports for type only imports. We could try changing that, but then we change the load order semantics of the code, which causes a lot of problems with real word JavaScript code (believe me, I've tried it :-)).

Also yes, by my understanding ES6 require will cause a load at runtime.

@dreamofabear
Copy link
Author

Ok I see, you're comparing the behavior of TS -> JS with TS -> Closure-annotated JS. I see how this could be a problem for existing TS projects that want to add Closure Compiler by adopting tsickle.

The thing is that our project doesn't care about TS -> JS behavior. We currently write Closure-annotated JS and existing code already performs runtime loads for "type-only" files -- as long as import order is preserved, behavior should not change. We just want to switch to TS just for the developer productivity benefit.

How about a new configuration option to change this behavior for projects like ours? I think googmodule sounds like a good choice but open to suggestions. I suppose this is my original FR so we've gone full circle. 😄 Sorry for not explaining our use case more clearly first.

@dreamofabear
Copy link
Author

Sorry for the delay on this. Going to close this in favor of a new issue that clearly describes the FR. I'll try to find time to take this on but would also greatly appreciate help. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants