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

Switches CommonJS modules to use the module metadata map #3417

Closed

Conversation

ChadKillingsworth
Copy link
Collaborator

The metadata provides the module type of the imported module. Also adds module interop integration tests.

I added 2 failing tests that need fixed. Both seem to indicate a problem with ES Module rewriting and default properties.

Fixes #3360

The metadata provides the module type of the imported module. Also adds module interop integration tests.
Fixes google#3360
LINE_JOINER.join(
"var $jscomp$destructuring$var0 = module$i0.default;",
"var foo$$module$i1 = $jscomp$destructuring$var0.foo;",
"var b$$module$i1 = module$i0.bar();",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo? Shouldn't it be module$i0.default.bar?

@@ -262,11 +261,127 @@ public void testCrossModuleSubclass6() {
});
}

@Test
public void testCommonJSImportsESModule() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also fwiw I'm pretty sure I had nothing to do with this one since I didn't touch the CJS pass :)

I'll take a look at the others, but can you handle this one?

@blickly blickly added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Jun 11, 2019
@blickly
Copy link
Contributor

blickly commented Jun 11, 2019

Internal issue created http://b/135037612

@jplaisted
Copy link
Contributor

Please sync after ec31e72. I've fixed ES modules importing CJS modules.

@lauraharker
Copy link
Contributor

Hi Chad, are you still interested in merging this PR?

@brad4d brad4d added the customer issue customer question, problem, or pull request label Apr 3, 2020
@ChadKillingsworth
Copy link
Collaborator Author

I'll pick this up again some day, but I will start a new PR. This is too stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes customer issue customer question, problem, or pull request internal-issue-created An internal Google issue has been created to track this GitHub issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CommonJS compatibility does not work as expected
6 participants