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

Typescript build #247

Closed
wants to merge 11 commits into from
Closed

Typescript build #247

wants to merge 11 commits into from

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Aug 24, 2016

Fixes #158

@parisk this is the first run at the introduction of a TypeScript build system. Here is the overview of changes:

  • The Terminal object is now exposed as Xterm.Terminal.

  • Running npm run build will run the code through TypeScript to generate commonjs modules and then through browserify to squash the modules, watching can be added in the future via watchify.

  • CompositionHelper went through a full typing conversion, so use this as an example of what "real TypeScript" code looks like

    • Notice there's a Terminal interface at the bottom allowing strong typing without needing a reference to the Terminal object, eventually this would be moved to another file and implemented by Terminal in terminal.ts.
    • Fat arrow functions are used just like in ES6 to allow this to continue to be used.
  • Viewport was refactored into a class without types, so use this as an example of a bare-bones conversion.

    • Notice that any is used to indicate that the type could be anything which relaxed the TypeScript compiler around usage of that variable/param/etc.
  • Terminal was retained in a js file and pulled in via require, I made an attempt to do this conversion but the file is just so damn big I wouldn't be confident in the result even if it did work. Terminal would have to be slowly refactoring into several modules. Check out this branch for a preview of what that looked like. An example of why this is such a big effort is how messy the code is, the following for example weren't initialized at the top like most of Terminal's members:

    private colors;
    private textarea;
    private rowContainer;
    private context;
    private document;
    private body;
    private isMSIE;
    private isMac;
    private isIpad;
    private isIphone;
    private isMSWindows;
    private viewportElement;
    private viewportScrollArea;
    private theme;
    private helperContainer;
    private compositionView;
    private compositionHelper;
    private charMeasureElement;
    private viewport;
    private brokenBold;
    private _refreshIsQueued;
    private _fullRefreshNext;
    private cursorBlink;
    private termName;
    private debug;
    private title;
    private popOnBell;
    private visualBell;
    

    TypeScript will allow us to clean this up in an effective way as errors go off for unused/uninitialized variables.

  • Because Terminal is in a js file, the build command is a little messy at the moment, this would be simplified when it's fully converted.

  • xterm.js is the entry point which provides the TypeScript classes to the (JavaScript) Terminal

  • typings/require.d.ts is a "typings file" which is a TypeScript concept that provided type safety to external libs, in this case the require function. This can also be included via the typings npm module.

  • tsconfig.json specifies the compile configuration, for example you can specify the type of module (amd, umd, commonjs, es6, etc.) as well as input and output files/dirs.


These things would need to be followed up after this change:

  • Add tslint for linting
  • Add more type safety to ensure higher quality
  • Slowly pull out modules from terminal.js, eventually creating terminal.ts when everything is converted
  • Change the addons to TypeScript
  • Verify whether it's Terminal.loadAddon is still needed, if so make sure it works
  • Determine a good folder structure for modules
  • Minify the build to improve performance

@parisk
Copy link
Contributor

parisk commented Aug 26, 2016

I think that going with TypeScript is the best choice right now. I have some comments/questions though, before we get a decision on this:

  1. Can we fix the failing tests before moving on?
  2. "The Terminal object is now exposed as Xterm.Terminal".
    Is it necessary to break the previous API when importing? /cc @Tyriar
  3. The demo is crashing for me (after building with npm run build)

Would changing the addons directory first help out with this (#250)?

Demo crash screenshot

image

@Tyriar
Copy link
Member Author

Tyriar commented Aug 26, 2016

@parisk

  1. I wanted to hold off on any more work on this until a decision was made. I can look into fixing the tests
  2. It may actually be necessary, I was actually going to request this anyway as I don't believe we can create a TypeScript typings files when the library itself it a function. Meaning xterm.js is currently pretty much the only part of the vscode code base that is not type safe 😛
  3. Did the build command fail? Are you on OSX?

@parisk
Copy link
Contributor

parisk commented Aug 26, 2016

I am on SourceLair and the build command succeeded

[email protected]:/mnt/project$ npm run build

> [email protected] build /mnt/project
> rm -rf out dist && mkdir -p dist && tsc && cp src/terminal.js out/terminal.js && browserify out/xterm.js --standalone Xterm >
 dist/xterm.js

I wanted to hold off on any more work on this until a decision was made. I can look into fixing the tests

Hopefully tomorrow we will settle down on this and we will be ready to move on.

Would I help if I moved the addons directory into src in the meantime (#250)?

@Tyriar
Copy link
Member Author

Tyriar commented Aug 26, 2016

@parisk looks like it's failing for the same reason as the test is failing; weirdnesses with window and document. I'll look into moving addons in this PR.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 26, 2016

@parisk it might be wise to add dist to .gitignore and have the release script add and commit it during a release.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 26, 2016

Ok the tests should be passing now, I moved the addons over but kept them in js for now so the build step is just copying them over.

My main concern now is that I'm not sure the best way to have git ignore all changes within the dist directory. Avoiding committing these changes whenever we can will help make the commit history less noisy and easier to navigate. Some useful discussions here:

@131
Copy link

131 commented Aug 26, 2016

@parisk , if the typescript choice is settled, i'm now switching my browserify transform from babelify to tsify, /cc @Tyriar

@131
Copy link

131 commented Aug 26, 2016

Hmm, after diving a little into it, i changed my mind, i'll stick with the es6 version.

As i can see it now, the typescript version is kinda pointless (as the main Terminal.js is still in pure JS, and the overall typescript brings, for now, unnecessary complexity - we do not gain anything, for now)

I'll continue to work in the ES6 version and rewrite Terminal.js as ES6 module.
@Tyriar , @parisk , may i suggest you reconsider the typescript edition once it's done ? (it will then have a lot more of sense)

@131
Copy link

131 commented Aug 26, 2016

Of course, that means a deep refactoring of Terminal.js internals and brings us totally out of scope for the #158

@131
Copy link

131 commented Aug 26, 2016

Also, for the global "document" & "window" scope, this is accidental complexity and is "fixed" in my PR

Basicaly, Terminal should not depends on document scope at ALL, until been attached with terminal.open(domElement) , follow this and you 'll simplify both the tests & code.

@parisk
Copy link
Contributor

parisk commented Aug 26, 2016

@Tyriar maybe we should separate the directories for temporarily built files and distributed builds. What if we kept temporary built files in src/dist?

@parisk
Copy link
Contributor

parisk commented Aug 26, 2016

Please wait for a minute. I think I am preparing a quite simpler PR.

@parisk
Copy link
Contributor

parisk commented Aug 26, 2016

It seems that when I was referring to a minute, I meant 3 hours...

I started getting scary with the magnitude of these PRs, so I started a new smaller one (strictly less than 200 additions), in order to move things forward gradually, instead of all at a time.

Could you please take a look at #251?

@Tyriar Tyriar closed this Aug 31, 2016
@Tyriar Tyriar deleted the typescript_build branch February 3, 2017 05:05
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