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

Support SystemJS module output. #2616

Closed
mikehaas763 opened this issue Apr 4, 2015 · 25 comments
Closed

Support SystemJS module output. #2616

mikehaas763 opened this issue Apr 4, 2015 · 25 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@mikehaas763
Copy link

I do realize that the SystemJS loader can be used with AMD and CommonJS modules. Is there any interest in outputting directly to the system module format?

@chicoxyzzy
Copy link
Contributor

+1. would like to see System loader implemented

@mikehaas763
Copy link
Author

I've seen in the contributing guidelines that features need to be marked as "approved" before being accepted. I hope this one gets approved :) Just out of boredom I did fork and begin implementing SystemJS module output. You can check it out at https://github.com/mikehaas763/TypeScript/tree/systemjs

Please do leave me comments and suggestions on it.

There are several similarities to how AMD modules are created so I'm thinking I should find an abstraction for those similarities and use it from both module types.

@mikehaas763
Copy link
Author

The actual function that emits the module format is mostly just a copy of the amd one at this point. I'm currently using https://github.com/ModuleLoader/es6-module-loader/wiki/System.register-Explained as a specification of sort for how I need to actually create the module format.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 6, 2015

@mikehaas763 We are currently considering adding UMD (#2605) module support. so would be looking into systemjs as well.

@vladima has a change along the same lines as yours. it would be great if you can collaborate on getting systemjs PR ready.

@mikehaas763
Copy link
Author

@vladima what change is it that you're working on?

@mhegazy It'd be nice if the current UMD spec had support for system modules!

@vladima
Copy link
Contributor

vladima commented Apr 6, 2015

In my branch https://github.com/Microsoft/TypeScript/tree/systemRegister I've added initial support for system emit, hoisting of variable\function declarations. Pending work: rewriting exports, adding tests

@mhegazy
Copy link
Contributor

mhegazy commented Apr 6, 2015

@mikehaas763 you can send PRs against @vladima's branch; i think this would be a good way to collaborate on this. we need to discuss the proposal in the language design meeting (which we usually have weekly). it would be great if we have a PR ready as well.

@DanielRosenwasser DanielRosenwasser added the Suggestion An idea for TypeScript label Apr 10, 2015
@niemyjski
Copy link

+1

@mikehaas763
Copy link
Author

Looks like vladima has submitted his PR for this. So System.register modules should be coming soon! 😄

@mhegazy
Copy link
Contributor

mhegazy commented Apr 21, 2015

here is the PR: #2840

@jamiewinder
Copy link

Is this the only way to handle circular references in TypeScript at the moment? I understand that SystemJS supports circular references, but at the moment I often get errors in the class code if the base class comes from another module involved in a circular reference (because the base constructor hadn't been imported).

At the moment I'm using SystemJS and TypeScript together by using the CommonJS module output format.

Hopefully we get to see this in TS1.5 :)

@mhegazy
Copy link
Contributor

mhegazy commented Apr 23, 2015

@jamiewinder with the new ES6 module syntax supported by TypeScript 1.5 you can get most of your circular references issues done.

e.g.

import { two } from "./module2";

export function one(counter: number) {
    two(counter);
    console.log(`in one ${counter}.`);
}
import { one } from "./module1";

export function two(counter: number) {
    if (counter) {
        one(--counter);
    }
    console.log(`in two ${counter}.`);
}

two(3);

running

tsc --m commonjs module2.ts

and

node module2.js

results in:

in two 0.
in one 0.
in two 0.
in one 1.
in two 1.
in one 2.
in two 2.

The reason is these module aliases will be rewrite by the TypeScript compiler, so my module1.js looks like:

var module2_1 = require("./module2");
function one(counter) {
    module2_1.two(counter);
    console.log("in one " + counter + ".");
}
exports.one = one;

where all references to two is rewritten as module2_1.two.

The cases where you need systemJs support is cyclical references in the main module body (i.e. not wrapped in function and classes) and in practice these are really limited scenarios.

@jamiewinder
Copy link

I'm using the TypeScript 1.5 Alpha with the ES6 module format. This is an example from an issue I raised on the SystemJS repo (though I'm not sure it's a SystemJS issue):

mod-a.ts

import { ClassB } from './mod-b';

export class ClassA
{
  constructor()
  {
  }

  public isB(): boolean
  {
    return this instanceof ClassB;
  }
}

mod-b.ts:

import { ClassA } from './mod-a';

export class ClassB extends ClassA
{
  constructor()
  {
    super();
  }
}

index.ts

import { ClassA } from './mod-a';
import { ClassB } from './mod-b';

var a = new ClassA();
var b = new ClassB();

window.alert(a.isB());
window.alert(b.isB());

The result is an error in the TypeScript __extends function (for ClassB) where the prototype is being set to that of the base constructor function. This because the base constructor wasn't imported (was undefined).

To clarify, this is ES6 modules, using TypeScript 1.5-alpha to output them as CommonJS format modules, to then be imported with SystemJS. So it's difficult to say where the issue is!

EDIT: A reply to my issue on the SystemJS repo suggests that it's CommonJS that's responsible for this. So, again, I certainly hope we can see register support in the TS 1.5 release. 😄

@mhegazy
Copy link
Contributor

mhegazy commented Apr 24, 2015

I think the issue here is order of imports.. so modifying your index.ts, to:

import { ClassB } from './mod-b';
import { ClassA } from './mod-a';


var a = new ClassA();
var b = new ClassB();

console.log(a.isB());
console.log(b.isB());

node index.js

i get:

false
true

@mhegazy
Copy link
Contributor

mhegazy commented Apr 24, 2015

And for completeness, systemjs module output should handle this correctly.

@mhegazy mhegazy closed this as completed Apr 27, 2015
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Apr 27, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2015

Covered by #2840

@cmichaelgraham
Copy link

@mhegazy need help with compiler options, don't see System.register at top of generated .js files...

here's the gulp task (using @ivogabe gulp-typescript)

gulp.task('build-ts-system', function () {
  var tsResult =  gulp.src([
        './aurelia-dts/**/*.d.ts'
        ,'./animator-css/*.ts'
        // ...  omitted for brevity ... //
        ],
        {base: "."})// tsProject.src() // instead of gulp.src(...)
    .pipe(ts({
         typescript: require('typescript'),
         declarationFiles: false,
         noExternalResolve: true,
         target: "es6",
         module: "system",
         emitDecoratorMetadata: true
    }));

    return tsResult.js.pipe(gulp.dest('../dist/system'));
});

getting close on typescript aurelia port --> gulp build

folders generated by gulp-typescript build !!!!!!!!

issue is no System.register in system build

gulp task for system register

reported issue: #2616 (comment)

.js showing no System.register

cmichaelgraham added a commit to cmichaelgraham/aurelia-ts-port that referenced this issue May 8, 2015
@mikehaas763
Copy link
Author

@cmichaelgraham

That works for me (except I use require('typescript').ModuleKind.System. Using "system" should work too though because I believe it automatically maps strings to the underlying ModuleKind type. How did you install your local typescript in to node_modules? Are you using the latest master branch?

I don't target ES6, I just target the default (I believe ES3) so that maybe is causing an issue. If you have ES6 then what's the point of translating the modules from ES6 to ES5 System.register modules?

@ivogabe
Copy link
Contributor

ivogabe commented May 8, 2015

@cmichaelgraham Support for systemjs was added in gulp-typescript v2.7.0. It now reads all the options from the ModuleKind & ScriptTarget enums at runtime, so all module kinds are now supported.

@Lenne231
Copy link

Is it possible to get one bundled JS file with 'es5' as target and 'system' as module output using the out parameter? The only thing we need are System.register statements creating named modules in the output.

@mhegazy
Copy link
Contributor

mhegazy commented May 25, 2015

@Lenne231 currently --out does not support combining --module; in 1.6 this should be supported though.

@ericmdantas
Copy link

+1. I'm also interested in this.

Just out of curiosity, are you guys waiting for the stable 1.5 to release it?

@mhegazy
Copy link
Contributor

mhegazy commented May 26, 2015

yes. we usually lock down on the release a few weeks before it is out publicly. so stay tuned.

@ericmdantas
Copy link

Alright, thanks!

@dsebastien
Copy link

+1 for being able to generate a bundle containing n system modules, that'll be sweet!

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript