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

tsickle fails on RxJS due to "body of a goog.module cannot use throw" #420

Open
kylecordes opened this issue Mar 7, 2017 · 11 comments
Open

Comments

@kylecordes
Copy link
Contributor

With a lot of tweaking of the right config in the right place, this repo branch gets most of the way to tsickling RxJS:

https://github.com/kylecordes/tsickle-fiddling/tree/build-rxjs

I wish the steps to get here were smaller, of course, but it's quite easy to run, or just read on:

rxjs/util/root.js:11: ERROR - The body of a goog.module cannot use throw.
    throw new Error('RxJS could not find any global context (window, self, global)');
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That file, emitted by tsickle:

goog.module('rxjs.util.root'); exports = {}; var module = {id: 'rxjs/util/root.js'};
/**
 * window: browser in DOM main thread
 * self: browser in WebWorker
 * global: Node.js/other
 */
exports.root = (typeof window == 'object' && window.window === window && window
    || typeof self == 'object' && self.self === self && self
    || typeof global == 'object' && global.global === global && global);
if (!exports.root) {
    throw new Error('RxJS could not find any global context (window, self, global)');
}
//# sourceMappingURL=root.js.map

I'm not sure where to go from here, but if I make the assumption "the goal of tsickle is to take most valid TypeScript code and make it valid properly typed Closure code", then the above seems like a bug or missing feature in tsickle. With that fig leaf as an excuse to enter this as an issue... here's what I'm trying to figure out:

  • Any pointer in the right direction to work toward tsickling RxJS?
  • Might I need to rearrange RxJS to somehow avoid problematic throw?
  • Manually convert a module or two to get past this limitation?
  • Beg the Closure team to support throw inside goog.modules?
  • Make tsickle emit ES2015 modules with type comments, which Closure can (?) recently consume, perhaps without this throw limitation?
  • Decide instead that some libraries are too painful to tsickle, and consume them in Closure as ES2015 and without full typing?
@evmar
Copy link
Contributor

evmar commented Mar 7, 2017

RxJS has definitely been the most troublesome library for us, because it really pushes the edges of legal TypeScript. That said, it's a critical dependency of ng2 and we use it with tsickle within Google so it is possible to make it work.

For this specific issue it looks like we worked around this by locally patching RxJS to wrap the code in an IIFE:

// Workaround error: The body of a goog.module cannot use throw.
// Wrap in IIFE
(function () {
  if (!root) {
    throw new Error('RxJS could not find any global context (window, self, global)');
  }
})();

I think the proper fix would be to make tsickle introduce the IIFE itself, or to fix the Closure compiler to accept this code. I'm not exactly sure why it rejects it anyway.

@evmar
Copy link
Contributor

evmar commented Mar 7, 2017

I've contacted the Closure authors about why this is illegal; I'll get back to you with their response.

@kylecordes
Copy link
Contributor Author

When I take another swing at this (perhaps tomorrow) I'll work around this specific one with the IIFE, and see what pops up next.

@kylecordes
Copy link
Contributor Author

Further work finds that there are really only a few remaining problems!

https://github.com/kylecordes/tsickle-fiddling/tree/build-rxjs

1) Closure "The body of a goog.module cannot use throw."

Details of the warning:

rxjs/util/root.js:11: ERROR - The body of a goog.module cannot use throw.
    throw new Error('RxJS could not find any global context (window, self, global)');
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This repo has a workaround for this problem. It simply comments out the line of
code. That won't matter unless someone somehow finds a way to run the code in an
environment lacking any of those globals.

Proposed solution: rather than wait for the Closure team to possibly relieve
this limitation, simply put an IIFE around it upstream in RxJS.

2) Dependency problem with Observable.empty()

Consuming application code must do: import 'rxjs/add/observable/empty'; to
make .empty() available so Notification can be compiled by Closure. I believe
this indicates some difficulty in how dependencies are translated from
TypeScript to Closure - but it is surprising that this is the only symptom of
such a problem. Hopefully this means it is a minor problem.

I don't have a proposed solution for this one yet. I don't know if this problem
will grow as more of RxJS is used in a consuming application, or whether it is
just this one glitch that could be worked around easily. This one needs ideas
from someone deeply familiar with tsickle.

3) Array access TypeScript construct

The following construct appears in the TypeScript code:

export class Subscriber<T> extends Subscription implements Observer<T> {
  [$$rxSubscriber]() { return this; }

Tsickle passes the important bit through essentially unchanged, Closure warns about it:

rxjs/Subscriber.js:60: WARNING - Cannot do '[]' access on a struct
    [rxSubscriber_1.$$rxSubscriber]() { return this; }
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This one is an open question: Is this a mismatch of the type systems? A missing
feature in tsickle?

So there are a few problems, but only a few - on a pile of TypeScript code that probably stretches tsickle and Closure pretty well.

@evmar
Copy link
Contributor

evmar commented Mar 8, 2017

As promised, I asked about #1. It sounds like the error is related to the confusing behavior of throw at the top level with modules -- whether it aborts the current module or all JS processing. I think RxJS should not explicitly check for this case and just assume one of those globals exists. If all three don't exist then RxJS will fail anyway (due to the value it gets being undefined) which is like the throw.

#2 is one of those problems I alluded to about how RxJS is the hardest code for us. That "/add/" thing is a super weird pattern seen only here. If you have a specific error message it'd help diagnose it further.

#3 is using an ES6 computed name. See http://exploringjs.com/es6/ch_classes.html "Computed method names". I am not sure how this works in the Closure world.

@kylecordes
Copy link
Contributor Author

For "throw": I wonder if that project would accept a PR to either wrap it or remove it, or whether such a thing be set aside unless there is a consensus of interest in Closure compatible RxJS.

For the /add/ thing: I've been following this a long time. A good part of my "day job" is teaching people Angular, which typically means were also teaching them TypeScript and RxJS and the observable concept all on-the-fly. I've gotten pretty good at a comprehensive explanation of what the /add/ thing does and why it does it and why it is needed and why it might be able to go away in the future.

In my repo, I commented out the import line from the (nearly trivial) main.ts:

// import 'rxjs/add/observable/empty';

Then ran a build to get the following warning:

rxjs/Notification.js:93: WARNING - Property empty never defined on module$contents$rxjs$Observable_Observable
                return Observable_1.Observable.empty();
                                               ^^^^^

I spent some time trying to track down why this happens. I couldn't diagnose anything specific, other than that the dependency resolution and traversal is missing this specific link somehow. Of course if I imported "Rx" at the top this error goes away, but then I'm accidentally pulling in all of Rx which defeats the purpose of this exercise in the first place. It's possible that with some experimentation adding such an import at exactly the right place in the consuming code (rxjs/Notification.ts) might dodge the problem.

For the computed name: I wonder if there is some different way to achieve this that might at least dodge the Closure warning. I see it's been discussed: google/closure-compiler#1737

alexeagle added a commit to alexeagle/RxJS that referenced this issue Apr 11, 2017
Closure compiler does not allow this. We need this modification to use RxJS in Angular projects.

Addresses angular/tsickle#420
@alexeagle
Copy link
Contributor

The rxjs/add/* modules monkey-patch the Observable or Operator to add methods. They patch both the values and the types.
So TypeScript's type-checker should ensure that calling Observable.empty is only allowed if the program has the rxjs/add/observable/empty imported somewhere.

@alexeagle
Copy link
Contributor

What is your motivation for wanting to run rxjs through tsickle? They already have local testing within the repo that their code is closure-compatible.

@kylecordes
Copy link
Contributor Author

@alexeagle My initial motivation was to see whether any better (smaller) result was possible from Closure, if provided with a version of that has Closure JSDocs. I have not looked at their test for compatibility, but as I recall their JSDocs are not valid JSDocs, Rather they are only even accepted it all by Closure if you let it spew a bunch of warnings but then disable seeing those warnings. Therefore my second motivation was to try to get more of the stack of tools to compile cleanly without warnings (and not just by hiding them).

This is probably a pointless windmill to tilt at though. I had done so simply because Angular only has this one major dependency, responsible for a meaningful chunk of compiled output size. So lacking knowledge of how much it might output size, I was hoping to find out by making it work.

(I assume that correct Closure full compatibility with types in JSDocs, enables more complete Closure optimizations, and what it can do if it sees and ignores a bunch of warnings.)

alexeagle added a commit to alexeagle/RxJS that referenced this issue Apr 12, 2017
1) don't throw at top-level scope.
Closure compiler does not allow this if a goog.module statement is present in the file. We need this modification to use RxJS with angular/tsickle.
Addresses angular/tsickle#420

2) refactor the conditional logic for finding the root object
See alexeagle/closure-compiler-angular-bundling#15
Closure seems to statically reduce the existing code and eliminates the conditional, making it always throw.
@alexeagle
Copy link
Contributor

We don't run closure compiler with type-aware optimizations yet. We'll have to have full tsickle-generated typing when/if we do that. We're still figuring out how to roll that out internally.
We can find more optimization opportunities within RxJS, that's true. We already know that we save a bit by using ES2015 rather than ES5...

benlesh pushed a commit to ReactiveX/rxjs that referenced this issue Apr 12, 2017
1) don't throw at top-level scope.
Closure compiler does not allow this if a goog.module statement is present in the file. We need this modification to use RxJS with angular/tsickle.
Addresses angular/tsickle#420

2) refactor the conditional logic for finding the root object
See alexeagle/closure-compiler-angular-bundling#15
Closure seems to statically reduce the existing code and eliminates the conditional, making it always throw.
@tadeegan
Copy link
Contributor

For 3) it looks like CC could potentially handle this situation since the Symbol is a const but it doesn't for now:

https://closure-compiler-debugger.appspot.com/#input0%3Dconst%2520s%2520%253D%2520'%2540%2540rxSubscriber'%253B%250A%250Aclass%2520A%2520%257B%250A%2520%2520%255Bs%255D()%2520%257B%2520return%2520this%257D%250A%257D%250A%250Aconsole.log(new%2520A().asdf)%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_SYMBOLS%3D1%26MISSING_PROPERTIES%3D1%26TRANSPILE%3D1%26CHECK_TYPES%3D1%26COMPUTE_FUNCTION_SIDE_EFFECTS%3D1%26FOLD_CONSTANTS%3D1%26DEAD_ASSIGNMENT_ELIMINATION%3D1%26INLINE_CONSTANTS%3D1%26INLINE_FUNCTIONS%3D1%26INLINE_VARIABLES%3D1%26FLOW_SENSITIVE_INLINE_VARIABLES%3D1%26INLINE_PROPERTIES%3D1%26REMOVE_DEAD_CODE%3D1%26EXTRACT_PROTOTYPE_MEMBER_DECLARATIONS%3D1%26REMOVE_UNUSED_PROTOTYPE_PROPERTIES%3D1%26REMOVE_UNUSED_VARIABLES%3D1%26COLLAPSE_VARIABLE_DECLARATIONS%3D1%26COLLAPSE_ANONYMOUS_FUNCTIONS%3D1%26COLLAPSE_PROPERTIES%3D1%26DEVIRTUALIZE_PROTOTYPE_METHODS%3D1%26REWRITE_FUNCTION_EXPRESSIONS%3D1%26DISAMBIGUATE_PROPERTIES%3D1%26AMBIGUATE_PROPERTIES%3D1%26PROPERTY_RENAMING%3D1%26OPTIMIZE_CALLS%3D1%26OPTIMIZE_PARAMETERS%3D1%26OPTIMIZE_RETURNS%3D1%26MOVE_FUNCTION_DECLARATIONS%3D1%26MARK_NO_SIDE_EFFECT_CALLS%3D1%26CROSS_MODULE_CODE_MOTION%3D1%26CROSS_MODULE_METHOD_MOTION%3D1%26CLOSURE_PASS%3D1%26PRETTY_PRINT%3D1

I think a patch to the compiler is possible for this.

I honestly don't see the point of this code:

export class Subscriber<T> extends Subscription implements Observer<T> {

  [rxSubscriberSymbol]() { return this; }

It seems like someone just wanted to use fancy computed properties/symbols. Can someone explain the use of this?

In general, computed properties will not work with CC well as it cannot statically analyze these.

For 2) I thought Typescript drops imports that are not used in source? For example type only imports. Perhaps I am wrong, just remember hearing this.

@evmar evmar changed the title "body of a goog.module cannot use throw" - can we tsickle RxJS? tsickle fails on RxJS due to "body of a goog.module cannot use throw" Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants