Skip to content

Conversation

@Tyriar
Copy link
Member

@Tyriar Tyriar commented Oct 1, 2016

A second much slimmer go at moving to TypeScript. Here's a summary:

  • JavaScript files are compiled using the --allowJs flag, this enables us to convert incrementally to TS
  • Typings are now tracked in typings.json, this is strong type information to recognize node for now (we won't need this when we convert fully though)
  • Didn't change the API

@parisk
Copy link
Contributor

parisk commented Oct 5, 2016

PR seems good in general, but I have a few comments and questions:

  1. Tests are failing, but I guess this should only require to build with TypeScript before running the tests
  2. Does this work also in browsers without a module system?

Last, I have to point out that while I am kind of skeptical of introducing TypeScript syntax into the core (possible barrier to entry for new contributors and found out tools like Flow that allow static type checking in pure JavaScript) the build system seems much better than the previous one which needed lots of different npm packages that do the job.

@Tyriar
Copy link
Member Author

Tyriar commented Oct 8, 2016

  1. Tests are actually passing fine locally, need to look into why it's failing on Travis
  2. TypeScript builds to es5/commonjs and then that goes through browserify similar to what we were doing before for a UMD wrapped file

Regarding Flow, that would be another way to get static typing but the power of it would only really unlock when you embrace explicit typing and other features of the language (which looks very similar to TypeScript). Regardless, they both look very similar to JavaScript and I'm not so sure worrying about potentially losing some contributions is worth avoiding type safe code.

bin/build Outdated
);
console.log(' OK.');
mkdir -p build
browserify src/xterm.js --standalone Terminal -p [ tsify ] --outfile build/xterm.js
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this solves most of problems with TypeScript building.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this, while it does support outputting as umd module types only amd and system are supported with the --outFile argument. They recommend using browserify or webpack for bundling umds.

@parisk
Copy link
Contributor

parisk commented Oct 31, 2016

Do we have any idea why tests might be failing on Travis?

@blink1073
Copy link
Contributor

It looks like you're running your mocha tests against ES6 code.

@Tyriar
Copy link
Member Author

Tyriar commented Oct 31, 2016

I'll sync with master and fix up the tests soon 😃

This change is largely just moving files with minor tweaks to them to fix,
the rest of the commit is build process changes:

- The addons/ and test/ dirs have been moved to src/
- The build directory has been removed
- TypeScript builds are output in out/, this is where tests are run
- The demo now relies on the dist/ build which is performed as part of ./bin/build
- Addons are now shipped under the ./build directory
@Tyriar
Copy link
Member Author

Tyriar commented Nov 11, 2016

@parisk since 2.1.0 is out, is it fine to merge Tyriar#1 into this PR and then into master provided the tests pass?

@parisk
Copy link
Contributor

parisk commented Nov 12, 2016

Sure @Tyriar, let's move on.

@Tyriar Tyriar merged commit 29cb576 into xtermjs:master Nov 12, 2016
@Tyriar Tyriar deleted the typescript_build_2 branch November 12, 2016 21:51
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

Successfully merging this pull request may close these issues.

3 participants