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

Supporting Common JS Named Exports using ES Module Imports #3308

Closed
zavr-1 opened this issue Mar 31, 2019 · 8 comments
Closed

Supporting Common JS Named Exports using ES Module Imports #3308

zavr-1 opened this issue Mar 31, 2019 · 8 comments
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 Mar 31, 2019

Hello & good day! Since the latest release, the import { version } from './package.json and other imports from JSON files is broken for Node.JS module resolution.

import { version } from '../package.json'
console.log(version)

Version: v20190301 Built on: 2019-03-05 23:14 with

-jar node_modules/google-closure-compiler-java/compiler.jar 
--compilation_level ADVANCED 
--module_resolution NODE
--js t/package.js package.json

produces console.log("0.0.1-alpha");

Whereas the update has now made it not possible :(

Exit code 1                                         
t/package.js:2: ERROR - variable null is undeclared
console.log(version)
            ^^^^^^^

1 error(s), 0 warning(s)

I'm now wondering if importing JSON files is actually part of Ecma6 but I don't see why it shouldn't be for at least for Node Module resolution in Closure.

If works with require and --process_common_js_modules btw:

const { version } = require('../package.json')
console.log(version)
@ChadKillingsworth
Copy link
Collaborator

This is by design. The interop between CommonJS and ES Modules is defined by NodeJS and has gone through several iterations (there's yet a new one coming). The last supported version stated that CommonJS modules only have a default export when referenced from an ES Module. CommonJS does not have named exports.

The latest proposal and work effort can be found at https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md

This is a complex issue and at this point I'd really like to see that this version will actually happen before implementing it in Closure-Compiler. Also - it's likely to include breaking changes so this could be a bit painful. The good news is that the Node community seems to be taking all the developer backlash to the last version to heart and is working hard to address that in the newest version.

@zavr-1
Copy link
Contributor Author

zavr-1 commented Apr 1, 2019

thanks

@ChadKillingsworth
Copy link
Collaborator

I realize the frustration here - it's pretty much felt by the entire dev community right now. We got here directly because a transpiler (Babel) made decisions that weren't actually able to be natively implemented.

However If Closure Compiler deviates from the current spec, we'll be making the problem worse - not better. Then when the spec gets figured out, we'll have to migrate our dev community to the proper way and that can be painful too.

Just as a note, the following should work just fine:

import pkg from '../package.json'
console.log(pkg.version)

It's only the destructuring style of import that has the issue.

@ChadKillingsworth ChadKillingsworth changed the title Advanced optimisation for json imports is broken since v20190325 Supporting Common JS Named Exports using ES Module Imports Apr 2, 2019
@blickly
Copy link
Contributor

blickly commented Apr 3, 2019

Created internal issue http://b/129802304, although it sounds like this is WAI to me.

@blickly blickly 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 Apr 3, 2019
@ChadKillingsworth
Copy link
Collaborator

It is - until Node figures out whether they are natively going to support CommonJS named exports, I don't want to support them in Closure Compiler. There is a strong desire to support these, but they don't know how to do it yet or what that would look like.

The full discussion is at nodejs/modules#264 but I can't really tell what the current state of things is by that issue.

@zavr-1
Copy link
Contributor Author

zavr-1 commented Apr 19, 2019

@ChadKillingsworth thanks for the advise about console.log(pkg.version), however

import packageJson from '../package.json'

throws now

java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.

INTERNAL COMPILER ERROR.
Please report this problem.

null
  Node(NAME packageJson): tt/t.js:3:12
console.log(packageJson)
  Parent(CALL): tt/t.js:3:0
console.log(packageJson)

  Node(SCRIPT): tt/t.js:1:0
import packageJson from '../package.json'
  Parent(ROOT): [source unknown]

        at com.google.javascript.jscomp.NodeUtil.newQName(NodeUtil.java:4014)
        at com.google.javascript.jscomp.ModuleRenaming.replace(ModuleRenaming.java:207)
        at com.google.javascript.jscomp.Es6RewriteModules$RenameGlobalVars.visit(Es6RewriteModules.java:867)
        at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:887)
        at com.google.javascript.jscomp.NodeTraversal.traverseChildren(NodeTraversal.java:999)
        at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:883)
        at com.google.javascript.jscomp.NodeTraversal.traverseChildren(NodeTraversal.java:999)
        at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:883)
        at com.google.javascript.jscomp.NodeTraversal.traverseChildren(NodeTraversal.java:999)
        at com.google.javascript.jscomp.NodeTraversal.handleScript(NodeTraversal.java:837)
        at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:862)
        at com.google.javascript.jscomp.NodeTraversal.traverse(NodeTraversal.java:371)
        at com.google.javascript.jscomp.NodeTraversal.traverse(NodeTraversal.java:381)
        at com.google.javascript.jscomp.Es6RewriteModules.visitScript(Es6RewriteModules.java:549)
        at com.google.javascript.jscomp.Es6RewriteModules.visit(Es6RewriteModules.java:378)
        at com.google.javascript.jscomp.NodeTraversal.handleScript(NodeTraversal.java:839)
        at com.google.javascript.jscomp.NodeTraversal.traverseBranch(NodeTraversal.java:862)
        at com.google.javascript.jscomp.NodeTraversal.traverse(NodeTraversal.java:371)
        at com.google.javascript.jscomp.NodeTraversal.traverse(NodeTraversal.java:381)
        at com.google.javascript.jscomp.Es6RewriteModules.processFile(Es6RewriteModules.java:186)
        at com.google.javascript.jscomp.Es6RewriteModules.hotSwapScript(Es6RewriteModules.java:175)
        at com.google.javascript.jscomp.Es6RewriteModules.process(Es6RewriteModules.java:164)
        at com.google.javascript.jscomp.PhaseOptimizer$NamedPass.process(PhaseOptimizer.java:328)
        at com.google.javascript.jscomp.PhaseOptimizer.process(PhaseOptimizer.java:237)
        at com.google.javascript.jscomp.Compiler.check(Compiler.java:1021)
        at com.google.javascript.jscomp.Compiler.performChecksAndTranspilation(Compiler.java:829)
        at com.google.javascript.jscomp.Compiler.lambda$stage1Passes$0(Compiler.java:759)
        at com.google.javascript.jscomp.CompilerExecutor$2.call(CompilerExecutor.java:102)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.

null
  Node(NAME packageJson): tt/t.js:3:12
console.log(packageJson)
  Parent(CALL): tt/t.js:3:0
console.log(packageJson)

        ... 32 more
Caused by: java.lang.NullPointerException
        ... 32 more

also I'm very sorry for my tone in the first comment, I do apologize I felt really terrible and ashamed of myself hence only now got to check the responses (function(rant): avoid is my signature 😝 ) I'm try to be a nice person but this modules thing along with babel really grind my nut. how am i supposed to program now?

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

? this is insane. on top of that no IDE supports that sort of programming, well at least VS Code but it I'm pretty sure nothing else does. if i'm trying to import something, I know what I'm doing don't i? what's the point of making the code looking crazy as well as destroying developer experience, just for the sake of some imaginary standard/babel? oh well i'm glad there were difficulties like that they make make new solutions and improve everything else that comes inbetween.
best regards,

@ChadKillingsworth
Copy link
Collaborator

I'll look into the crash and see what's causing that.

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')

Of course that's assuming the test module is a commonJS module:

module.exports.test = function(msg) {};

@zavr-1
Copy link
Contributor Author

zavr-1 commented Apr 25, 2019

@ChadKillingsworth well it's not been implemented correctly I'm afraid then because .default is what there is, and all named exports are set on the default.

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

console.log('requiring a common js from ecma:')
console.log(commonJs)
// common-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-js
module.exports = () => {
  console.log('default common js export2')
}
module.exports['named'] = () => {
  console.log('named common js export2')
}
-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 --package_json_entry_names module,main \
--entry_point /Users/zavr/depack/depack/example/commonjs/index.js \
--module_resolution NODE \
--js /Users/zavr/depack/depack/example/commonjs/index.js 
/Users/zavr/depack/depack/example/commonjs/common-js.js 
/Users/zavr/depack/depack/example/commonjs/common-js2.js

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);

Execution:

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

Also read more about fun with Babel here. This is how it can be used...

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

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

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

No branches or pull requests

3 participants