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

CommonJS compatibility does not work as expected #3360

Open
zavr-1 opened this issue May 3, 2019 · 15 comments
Open

CommonJS compatibility does not work as expected #3360

zavr-1 opened this issue May 3, 2019 · 15 comments
Assignees
Labels
internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation.

Comments

@zavr-1
Copy link
Contributor

zavr-1 commented May 3, 2019

The compiler puts each commonJs module into default property of the imported object.

// ecma
import commonJs from './common-js'

console.log('requiring a common js from ecma:')
console.log(commonJs)
CommonJS From ECMACommonJS From CommonJS
// common-js.js
const commonJs2 = require('./common-js2')

module.exports = () => {
  console.log('default common js export')
}
module.exports['named'] = () => {
  console.log('named common js export')
}

console.log('requiring a ' +
'common js from common js:')
console.log(commonJs2)
// common-js2.js
module.exports = () => {
  console.log('default common js ' 
+ 'export2')
}
module.exports['named'] = () => {
  console.log('named common js ' 
+ 'export2')
}

Run the compiler

java -jar /Users/zavr/node_modules/google-closure-compiler-java/compiler.jar \
--compilation_level ADVANCED --language_out ECMASCRIPT_2017 --formatting PRETTY_PRINT \
--process_common_js_modules --entry_point \
example/commonjs/index.js --module_resolution NODE --js \
example/commonjs/index.js example/commonjs/common-js.js example/commonjs/common-js2.js

Get the output

'use strict';
var a = () => {
  console.log("default common js export2");
};
a.named = () => {
  console.log("named common js export2");
};
var b = {default:() => {
  console.log("default common js export");
}};
b.default.named = () => {
  console.log("named common js export");
};
console.log("requiring a common js from common js:");
console.log(a);
console.log("requiring a common js from ecma:");
console.log(b);

Execute the output

requiring a common js from common js:
{ [Function: a] named: [Function] }
requiring a common js from ecma:
{ default: { [Function: default] named: [Function] } }

This does not make sense. I shouldn't write .default as confirmed here #3308. When it comes to Babel-compiled modules, it's a nightmare. Babel exports { default: code } itself, so the currently closure-compatible code becomes

import erte from '@a-la/fixture-babel'

console.log(erte.default.default())
console.log(erte.default.c())
console.log(erte.default.b())

And if you actually try to execute in with babel, you get

console.log(_.default.default.default());
                              ^

TypeError: Cannot read property 'default' of undefined

Developers developers developers developers
Babel Babel Babel Babel
Default Default Default Default

@brad4d
Copy link
Contributor

brad4d commented May 6, 2019

Created internal issue http://b/132070697

@brad4d brad4d added internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation. labels May 6, 2019
@ChadKillingsworth
Copy link
Collaborator

When you say "Try to execute with Babel", what do you mean?

The output you posted looks correct. I realize that the output is different than Babel, but that's intended.

@zavr-1
Copy link
Contributor Author

zavr-1 commented May 7, 2019

@ChadKillingsworth but you said in the last post this is incorrect. maybe modules internally be exported as { default: module }, but they shouldn't be IMPORTED like that? In case of babel that becomes { default: { default: module } } because with its transpilation achieves:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.default = exports.b = exports.c = void 0;

/**
 * A function that returns `erte`.
 */
const erte = () => {
  return 'erte';
};
/**
 * A function that returns `c`.
 */


const c = () => {
  return 'c';
};
/**
 * A function that returns `b`.
 */


exports.c = c;

const b = () => {
  return 'b';
};

exports.b = b;
var _default = erte;
exports.default = _default;

Therefore it's not correct the way it's been implemented!!!
Because if I wanted to use babel for my dev, i.e., runtime, testing, I would need to do just import erte from 'erte'; erte(). When I now want to use closure compiler I have to write import erte from 'erte'; erte.default.default() this breaks the compatibility between the two! I'm not gonna wait 1 minute to recompile the source each time I want to rerun the tests isn't therefore there's a need to use a transpiler for cheap transforms and only at the last stage the compiler. even if I don't use babel, I still have to do import commonJs from 'commonJs'; commonJs.default('test'); commonJs.default.named('test') and for sure it's not correct; it's what you said yourself

As to your example, that's not quite right. CommonJS has a default export (that's the ONLY export it has).

import test from 'test'
test.test('hello world')

@zavr-1
Copy link
Contributor Author

zavr-1 commented May 7, 2019

the compiler should call console.log(a.defaut); not console.log(a)!!! and in case of babel, there should logic to check it is an _esModule and call a.default.default or flatten the exported default! cheers

@ChadKillingsworth
Copy link
Collaborator

Closure Compiler transpiles and converts modules to scripts. It's not intended that the output be consumed as a module with proper imports or exports.

We've just begun discussing what output module support would look like. However, support will be concentrated on what is native to browsers and nodejs. Babel interop is really not a focus.

@zavr-1
Copy link
Contributor Author

zavr-1 commented May 7, 2019

@ChadKillingsworth please don't get me wrong you're in denial, the GCC currently breaks the commonjs compatibility and you don't want to accept it. I'm sorry I don't want to be rude but you're closing a real issue. I'm not talking about Babel support, I don't care for babel support I hate babel. They break JSDoc but nobody wants to notice it because everybody is hypnotised by money which is a trait of the current state of technology, that's why I'm starting neoluddite.dev where I want to get the power back to the people and not corporations such as facebook with their jests etc that take up 50mb just to install a testing framework https:/contexttesting/zoroaster#is-very-small-and-super-fast. The closure compiler is the tool that will help to really reform the JS today. I really only care for common-js compatibility. I'm telling you that I should not have to write

import commonJs from 'commonJs'
commonJs.default()
commonJs.default.name()

I should be able to write

import commonJs from 'commonJs'
commonJs()
commonJs.name()

and you've confirmed it yourself!!!
but it does not work!!! Without the advanced compilation, it works correctly. #3363 here you can see the module is called as console.log(module$a_or_b.default);, but in advanced mode, it is console.log(module$a_or_b); which does not work because module$a_or_b now contains an object with the default property rather than the function exported by the common js module.
What does it mean converts modules to scripts? If I want to import a module I should be able to do it regardless. There are tons of CommonJS modules that should just be able to be imported normally. It's not difficult. I will definitely look into it but I'm limited on the time resource at the moment. I don't need the code produced by the compiler to be a module, it can definitely be an executable script that imports other stuff from libraries. That's the whole point of modern JS.

@brad4d could you please comment on that? Where an i wrong? It used to work fine until April's release. Without this, the compiler is limited to pure ES6 or google required modules. Although I'm happy that it moves the scene forward, but it's definitely a bug at the moment.

Honestly all the respect is due for the hard work but I don't understand what's going on. I'm sorry this is an emotional post but there's definitely a lot of feelings involved, it's not just the question of common js, it's the question of the whole planet, and the difference between the good (simple, old-school) and toxic (corporate, money-centered) technology. Maybe it's hard to grasp at the moment but it will become evident and the choice you make today is really shaping the future tomorrow. And this issues is the most appropriate place to discuss it because if not here then where? Should I just be silent? Is it not what being part of the Open Source community means? To be able to raise concerns and receive some attention? I might be the only person seeing it but all ideas that change the world have to start somewhere. The one step that I want is a an explanation of why I cannot do

import commonJs from 'commonJs'
commonJs()
commonJs.named()

when I was able to do it a month ago.

@brad4d brad4d self-assigned this May 7, 2019
@brad4d
Copy link
Contributor

brad4d commented May 7, 2019

#3363 definitely looks like a problem because no ES6 modules are involved at all, yet 'default' is creeping in somehow.

I'll try to clarify Chad's statement:

Closure Compiler transpiles and converts modules to scripts. It's not intended that the output be consumed as a module with proper imports or exports.

It sounded like you were attempting to somehow treat the output of the compiler as a module and import it somewhere. That is not supported. If that's not what you were doing, then I really don't know what you mean by "Try to execute with Babel". Perhaps you could explain.

I am surprised to hear that this is a regression in behavior. The way @ChadKillingsworth described it to me yesterday it seemed like the only way ES6 import of a CommonJS module really could work, because ES6 has different export/import semantics. He is the primary maintainer of the CommonJS logic, but I suppose he could have been mistaken here, or else he had to change the interop behavior from what it was to fix some other problem.

I'm sorry you're feeling frustrated, but I assure you that our responses on this issue are only concerned with establishing whether this is a bug or not and explaining the behavior of the compiler. Module transpilation is a complicated thing to get right, and we would appreciate it if you could show more understanding an patience, especially if we're getting something wrong.

@ChadKillingsworth
Copy link
Collaborator

@brad4d I managed to create a minimal repo case.

@ChadKillingsworth
Copy link
Collaborator

test1.js

module.exports = 'a';

entry.js

import commonJs from './test1.js';
console.log(commonJs); // this should log 'a'

Actual output:
java -jar target/closure-compiler-1.0-SNAPSHOT.jar --process_common_js_modules --js test1.js --js entry.js

var module$test1={default:"a"};
console.log(module$test1);
var module$main={};

You'll notice that the ES Module is referencing the wrong property. That should most certainly be console.log(module$test1.default). It looks like something significant has changed in ES Module rewriting.

@ChadKillingsworth ChadKillingsworth removed their assignment May 8, 2019
@concavelenz
Copy link
Contributor

Do we know when this broke?

@ChadKillingsworth
Copy link
Collaborator

ChadKillingsworth commented May 8, 2019 via email

@jplaisted
Copy link
Contributor

jplaisted commented May 8, 2019

The only thing I've done recently around this is 92541e5. Maybe that will help your bisect. I don't see it as obviously breaking anything, but I would not rule it out.

In either case maybe the real action item here after this is fixed is to

  • Add unit tests to ensure future regressions don't happen
  • Make this code less brittle by ensuring ES and CJS modules have some common code path (right now they work by coincidence of that they happen to rewrite in similar ways)

@google google deleted a comment from jplaisted May 9, 2019
@ChadKillingsworth
Copy link
Collaborator

git bisect blames 0393c80

There are some commonJS integration tests, but looks like we are indeed missing some module interop integration tests: https:/google/closure-compiler/blob/master/test/com/google/javascript/jscomp/CommonJSIntegrationTest.java

@brad4d
Copy link
Contributor

brad4d commented May 17, 2019

@ChadKillingsworth can you provide such tests, since you're the one who knows how the CommonJS code is supposed to work?

ChadKillingsworth added a commit to ChadKillingsworth/closure-compiler that referenced this issue Jun 10, 2019
The metadata provides the module type of the imported module. Also adds module interop integration tests.
Fixes google#3360
@jplaisted
Copy link
Contributor

I believe ec31e72 has fixed ES importing CJS. The other way may still not be fixed.

@brad4d brad4d assigned ChadKillingsworth and unassigned brad4d Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-issue-created An internal Google issue has been created to track this GitHub issue triage-done Has been reviewed by someone on triage rotation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants