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

Introduce Babel as the build system, convert src/xterm.js to ES2015 and export EventEmitter #251

Merged
merged 2 commits into from
Aug 29, 2016

Conversation

parisk
Copy link
Contributor

@parisk parisk commented Aug 26, 2016

This PR introduces the following major changes in the codebase:

  • sets Babel (with the es-2015 preset) as the build system for xterm.js
  • converts src/xterm.js to an ES2015 native module
  • exports EventEmitter into its own module to demonstrate how this should work from now on

and eventually just closes #158.

Goals

  • lay the ground to break the core of xterm.js into multiple files to increase maintainability
  • make only the needed changes to achieve the above point and consequently keep this as reviewable as possible

Details

  • Babel is used in conjuction with browserify, derequire and exorcist in order to build the core into a standalone UMD module that can run in CommonJS, RequireJS and plain browser environment
  • bin/build is introduced to build xterm.js
    • regular builds go to build/ directory, which is ignored by Git
    • release builds go to the dist/ directory and are committed into the repository
  • bin/demo-server is introduced to server the demo
    • builds all files before starting
    • is being controlled by nodemon to auto-restart when a file changes (in the core or an add-on)

Known issues

  • The indentation level of xterm.js is totally wrong at this moment, after I removed the wrapping function, but I kept it as-is to maintain reviewability. This will be fixed in a next PR.
  • Addons are not pulled into their own directories, neither they got converted to ES 2015 in order to focus on as less things as possible in this PR. They still work with their old public API. This will be fixed in a next PR as well.

⚠️ Breaking changes

  • The constructor class of xterm.js is now accesses through xterm.Terminal. This is valid across all module systems (CommonJS, RequireJS and plain browser environment). This changed and now the Public API of Terminal stays the same.
  • Xterm is now imported through the dist directory instead of src

What I would like to achieve, but I did not manage to do is maintain the same public API for xterm, but there was no way I could get around this with browserify and UMD export.

Rollup worked great with this (before switching to babel), but it completely messed up the tests and I couldn't fix them.

This PR is greatly inspired by the great work done in #245 and #247 by @131 and @Tyriar.

While both of the above PRs are great, both of them included additional code and concepts that were not described in #158 (e.g. introducing TypeScript for type safety, or changing the communication protocol of the demo) and introduce complexity in reviewing of both PRs.

As a result I would like us to address all those topics in additional standalone issues, in order to be certain about our choices.

@parisk parisk added this to the 2.0.0 milestone Aug 26, 2016
This was referenced Aug 26, 2016
@131
Copy link

131 commented Aug 26, 2016

@parisk , would you please reconsider #245 if i just remove the changes in the demo app ?
I'll give more details in a few seconds

@131
Copy link

131 commented Aug 26, 2016

  1. using shell scripts will break windows build (there is no need for that, a npm task works just fine)
    (see package.json in my PR)

  2. Changing the public API (expose xTerm.Terminal) is un-necessary, as xterm (repository) is composed of proper ES6 modules, developpers wanting to access ViewPort will just need to do a require('xterm/utils/viewport')

  3. the change in the demo/app workflow is due to accidental complexity (introduced by @jerch AND acknoledge as it - see my PR ) having a single workflow - using excusive websocket fix a bug, as NON capable websocket will just leave terminal open forever.

@131
Copy link

131 commented Aug 26, 2016

In the last commit of my PR, i'v preserved the /src/ folder as it was in the master branch, so now #245 provides :

  • no api change
  • no directory structure change
  • es6 modularisation
  • windows build compatibility
  • (and the workflow in the demo app)
  • and a sane diff through all this : git diff -w master es6_remap src/xterm.js


# Watch for file changes inside the `src` and `addons` directories.
# Every time a file changes, then build xterm.js and restart the demo server.
node_modules/.bin/nodemon --watch src --watch addons --exec bash -c "./bin/build && node demo/app"
Copy link
Member

Choose a reason for hiding this comment

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

Any non-trivial scripts like this should be put directly in scripts for cross-platform compatibility.

@parisk parisk force-pushed the issue-#158-es2015 branch 3 times, most recently from 7b146d6 to edc3a25 Compare August 29, 2016 19:38
@parisk parisk force-pushed the issue-#158-es2015 branch from edc3a25 to ed1a31d Compare August 29, 2016 19:55
@parisk
Copy link
Contributor Author

parisk commented Aug 29, 2016

🆗 folks, this should be considered ready. Feel free to take a look, in order to merge this and move forward with further fixes.

@Tyriar
Copy link
Member

Tyriar commented Aug 29, 2016

Ship it 👍, I'll make sure it works in vscode in a few days to a week. We're going through our testing/release phase now so this will probably hit the Insiders build in a week or so.

@parisk parisk merged commit a9c85fe into master Aug 29, 2016
@parisk
Copy link
Contributor Author

parisk commented Aug 29, 2016

Awesome. I will move on with creating some issues to further improve the codebase after this.

@Tyriar Tyriar deleted the issue-#158-es2015 branch October 31, 2016 19:01
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.

Introduce some build system to enable more than one js file
3 participants