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

Improve CommonJS UMD pattern detection #2963

Closed

Conversation

ChadKillingsworth
Copy link
Collaborator

We have historically detected the Universal Module Definition pattern by looking for an export nested inside any if statement. However, this is far from correct. In CommonJS, exports can be conditionally created.

Improves the UMD pattern detection by requiring the if statement to reference the module or define names.

In addition, the pass now creates export aliases for any export nested inside an if or function. Rewriting such exports to avoid aliases potentially produced incorrect code.

Fixes #2841

Checks to see if the enclosing "if" test references the "module" or "define" names.
Prevents exports nested in IF clauses from being always detected as a UMD pattern when they may just be performing a conditional export.
@tjgq tjgq closed this in 9009ecb Jun 5, 2018
@ChadKillingsworth ChadKillingsworth deleted the commonjs-umd branch June 5, 2018 11:28
swannodette pushed a commit to clojure/clojurescript that referenced this pull request Jun 14, 2018
One important fix in the latest Closure-compiler is fix for UMD wrapper
detection which now correctly leaves React CJS bundle intact.

google/closure-compiler#2841
google/closure-compiler#2963
Yannic pushed a commit to Yannic/com_google_closure_compiler that referenced this pull request Jul 16, 2018
Checks to see if the enclosing "if" test references the "module" or "define" names.
Prevents exports nested in IF clauses from being always detected as a UMD pattern when they may just be performing a conditional export.

Closes google#2963

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=199227525
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.

2 participants