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

fix(closure): don't throw at top-level scope #2546

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

alexeagle
Copy link
Contributor

Closure compiler does not allow this. We need this modification to use RxJS in Angular projects.

Addresses angular/tsickle#420

Description:

Related issue (if exists):

@kwonoj kwonoj requested a review from benlesh April 11, 2017 15:21
Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks ok to me.

@alexeagle
Copy link
Contributor Author

Note, this is the minimal fix. You could instead just avoid throwing an exception: it's unclear from module semantics whether such a throw will abort execution of this one module, or the loading of the whole program. You'd be just as well with a runtime error when attempting to access the root property if it doesn't match one of the conditions above. (suggested by @evmar)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0003%) to 97.687% when pulling cafa477 on alexeagle:patch-1 into dc2e7f1 on ReactiveX:master.

@alexeagle
Copy link
Contributor Author

Hold on merging this please, it looks like there is still an issue with closure compiler in the newest release. I'll figure it out tomorrow...

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 Author

okay, fixed now @benlesh.
Exciting news: this is the only change needed to make Angular and Closure work, we don't need the ESM distro.

@coveralls
Copy link

coveralls commented Apr 12, 2017

Coverage Status

Coverage decreased (-0.04%) to 97.644% when pulling 78a0193 on alexeagle:patch-1 into dc2e7f1 on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Apr 12, 2017

Note, this is the minimal fix. You could instead just avoid throwing an exception: it's unclear from module semantics whether such a throw will abort execution of this one module, or the loading of the whole program

Yeah, overall this throwing seems odd (I probably added it). I've often wondered if we could get around this whole root module to begin with.

This LGTM tho, next patch version.

@benlesh benlesh merged commit 0ecf55d into ReactiveX:master Apr 12, 2017
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants