Switch to new ES6 style modules in Excalibur

[eonarheim]

Context

Currently, we are using the old /// <reference path="./path/to/my/module"/> syntax. This is old and clunky and it seems like VSCode has a hard time interpreting modules and dependencies.

Proposal

I propose we move to the new ES6 style module syntax (commonjs) that is now available in the TypeScript compiler so that dependencies look like this

We just need to set the module style in the compiler settings to commonjs

This will be a pretty large task since it will need to touch most all files in Excalibur

[kamranayub]

I wholeheartedly agree. It'll be better in the long run.

On Thu, Jun 16, 2016, 21:34 Erik Onarheim [email protected] wrote:

Context

Currently, we are using the old /// syntax. This is old and clunky and it seems
like VSCode has a hard time interpreting modules and dependencies.
[image: image]
https://cloud.githubusercontent.com/assets/612071/16138935/23313ed0-3408-11e6-8f93-8ce5686365a7.png
Proposal

I propose we move to the new ES6 style module syntax (commonjs) that is
now available in the TypeScript compiler so that dependencies look like this

[image: image]
https://cloud.githubusercontent.com/assets/612071/16138969/77a5b75c-3408-11e6-8b7a-5cb1775d0a3f.png

We just need to set the module style in the compiler settings to commonjs

This will be a pretty large task since it will need to touch most all
files in Excalibur

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#606, or mute the thread
https://github.com/notifications/unsubscribe/AAiaa1j9hQw856IlMzeHMvoJ6aGKasFVks5qMgfKgaJpZM4I3-0H
.

[soncodi]

Is this already being worked on? If not, I can send a PR.

[eonarheim]

Hi @soncodi, this is not yet being worked on. However, this will be a very large and difficult change as it will touch most every file in the Excalibur code base and require changes to the build process.

If you'd like to contribute to Excalibur, we recommend looking at issues labeled "jump-in" to help familiarize yourself with developing for this project. Please review our contributing guidelines.

A few things that you will run into if you work on this issue:

• The TypeScript compiler won't emit a single file if you go to ES6 style modules (a single file is definitely something we want).
• After adding ES6 module update you may need to run the code through another post process to combine all the modules into a single file (maybe browserify? or babelify? more research is needed).

[kamranayub]

As mentioned in #675 could we add the required lines to add compatibility to module loaders as a quick stopgap?

[kamranayub]

I might take this on during holiday break in the next couple weeks for 0.9.

[guahanweb]

Is there a shared branch we can set up to work collectively on this task? There are a ton of files to change, and I'm happy to help with some, but I don't want to double up efforts.

[kamranayub]

I was going to create a branch yesterday but didn't yet--if you want to create one, I was going to go with 606-es6-modules. I could publish that today if we want. It does depend (slightly) on #720 being merged first so we are all updated to 2.0

[guahanweb]

If we're confident that #720 will be merged, I'll just create a branch on my fork (named as you recommend) and go ahead and merge that locally. I'll submit the new branch as a PR with a note indicating dependency on #720 getting merged first.

[guahanweb]

@kamranayub, I've been looking into this, and there's not going to be an easy way to break it up. As soon as you change one module to an export or namespace, references down the inclusion stack are all broken. Additionally, as soon as you declare a single import, it seems to invalidate the other triple slash refs. It seems that this is going to just have to be one massive labor of love to change all declarations at once...

Unless someone else knows of a way to use both an import and triple slash reference in the same file? I've been striking out.

[kamranayub]

Yeah, that's why I was going to handle doing this in one fell swoop sometime next week ;) But it's cool you took a swing at it! Don't feel like you have to tackle this bad boy, there are other easier issues that are up for grabs too.

[guahanweb]

@kamranayub, yea, I was just hoping to find a way to be able to do it incrementally. I've led projects with massive changes like this before, and unless they can be automated to help minimize merge conflicts, they're quite a pain. I'll take a look and see what else is outstanding.

[kamranayub]

There's another wrinkle with this issue.

While it will be possible for us to switch to a module system and still output to a single file, one outstanding issue is that the generated declaration files are not "flattened" meaning that the generated concatenated declaration file may expose internal/private modules. It's not a dealbreaker but it is annoying.

There's an open PR for it: microsoft/TypeScript#5332

[kamranayub]

OK, I'm about ready to throw in the towel.

Here are the problems converting the entire codebase to modules:

• Loss of namespacing. In order to restore some of the previous namespaces like ex.Internal we would need to re-export anything we wanted to expose in those namespaces. Doable but annoying.
• Force export of unused modules. All the bundling tools read the dependency graph of the project to build an optimized bundle. However, we also have stuff that is useful to expose publicly (e.g. sprite effects) that aren't necessarily used within the core library. Again, doable--but annoying.
• Circular Dependencies. This is the dealbreaker I feel. It's well-documented that CJS, AMD, and ES6 modules don't handle circular dependencies very well, especially for class-based architectures like Excalibur due to the way TS generates the output JS. There are some super ugly hacks that maybe will work. The other option is to re-architect or rewrite the portions of code that cause the circular dependency issue. The best part is--you won't know until you hit the browser because TS/compiler is totally cool with circular dependencies, which makes this extra ugly.

I think that we could possibly move to a module syntax--in fact, I've converted the entire engine over in my branch and everything compiles. But due to this weird circular dependency issue, it's going to be a bear to fix and it will be a breaking change.

@excaliburjs/core-contributors I don't know what you guys want to do. On one hand, it might be nice to fix the smells of circular dependent code but on the other, it's all over the codebase because Ex is a global library.

We could wait for 1.0 and do a major re-tool of the codebase, doing modules in the process. We could throw in the towel and keep using an internal module like we are now.

I could also try just cleaning up the triple-slash references--I might be able to use the tsconfig or a single _references file to get things working and clean up the code.

The goal of doing this was to clean up the code but it ends up complicating the build process immensely--there's no way to avoid adopting a bundler, whether that's RequireJS + Almond, SystemJS + JSPM, Webpack, or Browserify.

In the end, we just want to make it easier to use Excalibur in a module environment--so maybe instead we should just focus on creating templates for the common use cases we see from folks (Angular, Webpack, CommonJS, etc.) and provide the right guidance. I could also easily add a UMD header to the output JS to support CJS and AMD module loaders.

[kamranayub]

UPDATE

I got it working!

Turns out the circular dependency rabbit hole was not deep at all--the only problematic class was Actor because it had a few lines that depended on its sub-classes UI Actor, Trigger, and Label. I think we can eliminate that dependency, I have a proposed fix. I also was able to re-export the entire public API without too much hassle in a set of Index.ts files.