Skip to content

closure fails on tsickle output [on a simple test case] #374

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

Closed
orenmizr opened this issue Feb 15, 2017 · 14 comments
Closed

closure fails on tsickle output [on a simple test case] #374

orenmizr opened this issue Feb 15, 2017 · 14 comments

Comments

@orenmizr
Copy link

orenmizr commented Feb 15, 2017

latest compiler[^20170124.0.0] & tsickle[^2.1.6] (please help @mprobst @evmar @petebacondarwin @alexeagle @vikerman )

error
build/testScript.js:2: ERROR - Required namespace "build_testDep" never defined.
var testDep_1 = goog.require('build_testDep');

command tsickle:
"./node_modules/.bin/tsickle" --externs=build/externs.js

// tsconfig

{
  "compilerOptions": {
    "target": "es5",
    "noImplicitAny": false,
    "sourceMap": false,
    "outDir": "build",
    "module": "commonjs",
    "noEmitHelpers": false
  },
  "include": [
    "src/test*.ts"
  ],
  "exclude": [
  ]
}

command compiler:
java -jar ./node_modules/google-closure-compiler/compiler.jar --compilation_level ADVANCED --js build/*.js --js_output_file dist/p.js --output_wrapper "!function(){%output%}.call({});

// testScript.ts

import {Dep} from "./testDep";

class Test {
    value: Dep;
    constructor(val: Dep) {
        this.value = val;
        alert('still testing');
    }
}

var t: Test = new Test(new Dep(1));
alert('i am compiled!');

(after tsickle)

goog.module('build.testScript'); exports = {}; var module = {id: 'build/testScript.js'};
var testDep_1 = goog.require('build_testDep');
var Dep = testDep_1.Dep; /* local alias for Closure JSDoc */
var Test = (function () {
    /**
     * @param {!Dep} val
     */
    function Test(val) {
        this.value = val;
        alert('still testing');
    }
    return Test;
}());
function Test_tsickle_Closure_declarations() {
    /** @type {!Dep} */
    Test.prototype.value;
}
var /** @type {!Test} */ t = new Test(new testDep_1.Dep(1));
alert('i am compiled!');

// testDep.ts

class Dep {
    value: Number;
    constructor(val: Number) {
       this.value = val;
    }
}

export {Dep};

(after tsickle)

goog.module('build.testDep'); exports = {}; var module = {id: 'build/testDep.js'};
var Dep = (function () {
    /**
     * @param {!Number} val
     */
    function Dep(val) {
        this.value = val;
    }
    return Dep;
}());
exports.Dep = Dep;
function Dep_tsickle_Closure_declarations() {
    /** @type {!Number} */
    Dep.prototype.value;
}

@orenmizr
Copy link
Author

i consider this a simple "hello world" test case. if anything missing please tell me

@orenmizr orenmizr changed the title closure fails on tsickle output closure fails on tsickle output [on a simple test case] Feb 16, 2017
@evmar
Copy link
Contributor

evmar commented Feb 16, 2017

This is where we emit the goog.module

this.emit(`goog.module('${moduleName}');`);

where pathToModuleName is

export function pathToModuleName(context: string, fileName: string): string {

So I'm not sure why there's a '.' in there.

@orenmizr
Copy link
Author

my friend (@evmar) I don't follow you. I gave the entire source and versions. it's two files. are you saying this is a bug?

@evmar
Copy link
Contributor

evmar commented Feb 16, 2017

Yes, I think it's a bug. I was just dumping some notes about the relevant code. I'm not sure why it's misbehaving here.

@orenmizr
Copy link
Author

thanks for clarifying. isn't this tool widely used by the angular community ? (weird that it failed to figure out one dependency). any workarounds / suggestions ? i am closure depended.

@evmar
Copy link
Contributor

evmar commented Feb 19, 2017

It's still a work in progress. We use it heavily within Google but our environment is different and doesn't cause this bug for whatever reason.

@orenmizr
Copy link
Author

i realize this is work in progress (and very appreciative of it). however, if this utility isn't a standalone and only tested for angular setup environments - please state it in the docs. i was scratching my head for countless hours. I might not be the only one.

do you need me to change to headline to be descriptive of the bug ?

@orenmizr
Copy link
Author

orenmizr commented Feb 22, 2017

@evmar my colleague tal obsfeld pointed out to me (and might be right):

The main issue is that there is a '.' instead of '' ?
If so, then the root cause is the regex in pathToModuleName function.
"build/testDep.".replace(///g, '.').replace(/^[^a-zA-Z_$]/, '
').replace(/[^a-zA-Z0-9._$]/g, '_');
and you'll get build.testDep

They are replacing '/' on purpose to '.' for some reason
The reason is "Module identifiers may not contain “/”, “\” or begin with “.”"

I think the bug is actually with the output for testScript.ts line 2:
var testDep_1 = goog.require('build_testDep');
var testDep_1 = goog.require('build.testDep');

My theory is that because we are running it on window, the file name is reported as 'build\testdep'. If you run the regex in pathToModuleName with this input, you'll get a '_' instead o '.'...

If we can get it to run in linux then we are good, because we can configure the team city build to run the build on a linux agent machine

@evmar
Copy link
Contributor

evmar commented Feb 22, 2017

Great guess! You are probably the first person to try this on Windows. :)

@orenmizr
Copy link
Author

orenmizr commented Feb 22, 2017

this tool is a must so we ran it on linux as well and got this result

ERROR - The body of a goog.module cannot reference this.
var __extends = (this && this.__extends) || function (d, b) {

we did the emithelper flag and got: ERROR - variable XXXX is undeclared (and) ERROR - variable __extends is undeclared

closure simply refuses to compile tsickle output.
help ? directions ? hints ? (will be much appreciated)

@evmar
Copy link
Contributor

evmar commented Feb 22, 2017

Thanks, I filed separate issues about the problems you've uncovered.

@orenmizr
Copy link
Author

i appreciate your responsiveness. two questions:

  • it sounds like this tool is very much tailored for angular2 (and it's idea of modules i suppose). has anyone tried tsickle-ing a basic typescript (angular free) project and approved the output ?

  • not related: is there anything like that kind of compression for es6 projects ? :)

@kylecordes
Copy link
Contributor

The tsickle source, in the test_files directory, contains numerous small non-Angular TypeScript programs which compile with tsickle via its test suite.

@PeterHorvathIsento
Copy link

As an important help to the Angular developers, I would like to mention:

this happens in the tsickle-compiled .js files, if there is a TypeScript-decoration in the original typescript.

This this makes the google closure compiler unhappy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants