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 some build system to enable more than one js file #158

Closed
Tyriar opened this issue Jul 4, 2016 · 26 comments · Fixed by #251
Closed

Introduce some build system to enable more than one js file #158

Tyriar opened this issue Jul 4, 2016 · 26 comments · Fixed by #251
Assignees
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jul 4, 2016

It's difficult to simplify the code base if it all sits in a 5000 line file.

@parisk
Copy link
Contributor

parisk commented Jul 5, 2016

I totally agree with that. Let' get on top of this after finishing documentation and releasing 1.0.0.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 3, 2016

I suggest webpack (prototype here) as I've used it before and it does exactly what we want (bundles several files into 1 using require()). I've also barely touched the surface of its capabilities as well and it's very popular.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 19, 2016

Another option is TypeScript which I've become quite fond of after working on vscode. This would allow multiple files to be built into one and provide type safety which would promote code quality (especially good since unit tests are patchy).

@parisk
Copy link
Contributor

parisk commented Aug 22, 2016

I think that plain ES6 or TypeScript are more suitable for this. Webpack seems to be more of an overkill at this moment.

The most important thing that we have to take into account is how are going to ship the code here. This most probably will introduce breaking changes (especially for Bower users), since we will need to find a way to ship the built artifacts to keep this library working in the browser out of the box.

I am adding this to the "open" milestone for version 2.0 and start some experimentation on this, preferably with plain ES6, which is close to TypeScript.

My main concerns with TypeScript are:

  1. barrier to entry for new contributors
  2. none of the rest maintainers has worked with TypeScript in the past (if I can recall right), me included

@parisk parisk added this to the 2.0.0 milestone Aug 22, 2016
@Tyriar
Copy link
Member Author

Tyriar commented Aug 22, 2016

When using bower most projects with a build system add the built files to a dist directory and those are the ones you link to. It would introduce a breakage though so v2 would be appropriate.

@jerch
Copy link
Member

jerch commented Aug 23, 2016

+1 for using Typescript. No clue about the current state of ES6 support, I think generators are still at an early stage in some JS engines and therefore not usable yet.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 23, 2016

I'll assign myself to put together a TypeScript prototype.

@Tyriar Tyriar self-assigned this Aug 23, 2016
@131
Copy link

131 commented Aug 23, 2016

May i suggest you start with ES6 & Babel ? Leveraging typescript is a good thing, yet, it will come with a price.

Having an es6 project will allow your user to just "var Terminal = require('xterm')" and use the raw source code (browserify will work just fine)". When with typescript, you'll have a different file for the build and the source of your application.

Also, thanks to typescript design, an ES6 project is, de factor, compatible typescript. The full migration to typescript can come in a second (and very smooth) step.

May i help here ?

@Tyriar
Copy link
Member Author

Tyriar commented Aug 23, 2016

Sourcemaps gets around that if needed though, the type safety is the main benefit imo.

@131
Copy link

131 commented Aug 23, 2016

This is not about the sourcemap, but having no "build" step required for thoses who just use browserify (ES6 is already supported in most modern browser)

@Tyriar
Copy link
Member Author

Tyriar commented Aug 23, 2016

@131 due to bower we would likely be committing the compiled version to a dist or out directory.

@131
Copy link

131 commented Aug 23, 2016

Is it ok for me to give a try to a es6 version ? (i played a little)
with a bower compiled output in /dist , of course !

@jerch
Copy link
Member

jerch commented Aug 23, 2016

The most promising changes in ES6 cannot be polyfilled by babel - the iterator/generator/comprehension concepts. Downvote for the vanilla ES6 idea, because of the current state of ES6 in commonly used JS engines. And still hoping for some optional type annotations in some later JS versions...

@131 131 mentioned this issue Aug 23, 2016
@131
Copy link

131 commented Aug 23, 2016

Babel support for generator is complete (babel preset es2015) and works perfectly with with co, yet, current code do not gain much of those.

I've a branch ongoing #245

As i'v ditched fetch, no promise are left for now (so it's hard to do a demo of generators), but if you want, i can easily set up a working version with generator & promise (just tell me on what event / operation )

@parisk
Copy link
Contributor

parisk commented Aug 23, 2016

Okay, so let me contribute to this.

First of all let's agree that in version 2.0 (after we migrate to either option):

  1. the public API for importing this library will break
  2. we will ship a browser-ready monolith for the core and browser-ready files for the add-ons

These are prerequisites to make usage of this library as easy as possible, while making it more maintainable.

Next, let me answer to as many comments as I can:

About Generators

I think generators are still at an early stage in some JS engines and therefore not usable yet.

@jerch as @131 mentioned, generators are fully supported in the babel polyfill: http://babeljs.io/docs/learn-es2015/#generators. This does not concern us in any case though, since we are not using generators right now in any way and there is no open issue that could benefit from generators. So either way we won't be using them any time soon (until we have to).

About importing (TypeScript vs ES6)

Having an es6 project will allow your user to just "var Terminal = require('xterm')" and use the raw source code (browserify will work just fine)". When with typescript, you'll have a different file for the build and the source of your application.

Sourcemaps gets around that if needed though, the type safety is the main benefit imo.
@131 due to bower we would likely be committing the compiled version to a dist or out directory.

@131, I agree with @Tyriar here. Anyone who will consume xterm.js will have access to the built files since they will fetch a release of a library published in npm, bower or GitHub with the built artifacts included.

About TypeScript vs ES6

While personally I am not settled yet on which is the best solution, I lean slightly towards vanilla ES6 for the following reasons (most of them mentioned in #158 (comment)):

  • Except for @Tyriar none of the maintainers has ever worked with TypeScript.
  • TypeScript could introduce a barrier to entry for (new) contributors.
  • TypeScript might be an overkill for what this issue attempts to solve. The current issue is to "Introduce some build system to enable more than one js file". This can be achieved with plain ES6 and Babel and nothing more. While type safety is the biggest benefit of TypeScript, I am not sure yet if this code base is in need of type safety.

Despite the above arguments though, I am really open to moving to TypeScript, since building a code base that is reliable and future proof is always a good idea. We just have to weight the pros and cons.

Last, as I have no significant experience with ES6 and TypeScript, I would ❤️ to see both prototypes and contribute to the conversation of both. So thanks a lot both @Tyriar and @131 for offering to create prototypes 🎉 .

P.S.: I started reading about TypeScript and ES6 already, in order to provide some more constructive comments and help take a proper decision.

@131
Copy link

131 commented Aug 24, 2016

I'v updated my PR, with a working /dist/ folder (with a working client side demo in it, no server required). , please look at the repository tree (the PR is not so nice to read). Addons are also available.

npm run start for server side demo is also working.

Regarding @parisk, for 1: the public API for importing this library will break
We can still make a very strategic move and have, before the "/dist/" folder renamed "/src/" , so we wont break nothing.

@Tyriar , @parisk , @jerch i guess i'm done, now, i'll happily have your feedback !

@Tyriar
Copy link
Member Author

Tyriar commented Aug 24, 2016

On addons, I think they should be split out to other repos in the xtermjs org, this makes including them easier (install the dep and require it), promotes separation and makes them more clear examples on how to write an addon.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 24, 2016

Having said that, they currently use private APIs so if this was the case we should expose public APIs that enable them to do this sort of stuff.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 24, 2016

TypeScript PR is up at #247

@jerch
Copy link
Member

jerch commented Aug 25, 2016

Ah well, my info about the generator support of babel seems to be outdated. Why I came up with it - generators as coroutines are a nice way to work with streamline data. This technique is often used for code separation in Python. (I think the generators in ES6 are heavy influenced by Python.)

xterm.js could benefit from this by splitting the Terminal object into pieces of responsibilities. Especially the parser could be separated from terminal state handling easily with a generator pattern. It would reduce the fat switch statements in Terminal.write which is hard to maintain at the moment. Of course this comes at a price - the generator is likely to run somewhat slower due to the additional overhead of handling the coroutine state.

To illustrate this further, the way Terminal.write does the data handling is:

for char in data
  switch char
    adjust parser state if needed
    with parser state
      terminal_state_handling
    end
  end
end
trigger event

What makes this hard to maintain is the deep nesting of the terminal state handling within the parser states in one big method. Also it is not guaranteed that on a deeper level the parser state gets not modified (it actually happens).

With generators this can be rewritten to:

parser:
for char in data
  switch char
    adjust parser state if needed
    yield [parser state, char]
  end
end

terminal:
for [parser state, char] in parser
  with parser state
    terminal_state_handling
  end
end

This can be even more simplified if the parser aggregates the ANSI codes to basic actions a terminal has to support (internal API) and leads to a complete separation of ANSI code parsing and terminal handling:

parser:
for char in data
  switch char
    adjust parser state if needed
    if action endpoint
        yield [action, aggregated data]
    else
        aggregate char
  end
end

terminal:
for [action, aggregated data] in parser
  call action(aggregated data)
end

With a well defined set of actions the parser and the terminal can be developed and tested independently. Since a full escape code parser model with actions got already specified elsewhere (http://vt100.net/emu/dec_ansi_parser) it is quite easy to adopt it.

@131
Copy link

131 commented Aug 25, 2016

Hi @jerch, thanks to your advices, i'm willing to give a try, i'll get back to you soon

@jerch
Copy link
Member

jerch commented Aug 25, 2016

@131 Oh ok, this will be a major rewrite of Terminal.write and is somewhat offtopic here, since it addresses a specific rewrite idea and not the build system in particular. Maybe a v2.0 rewrite issue is needed to track all the leftover ideas - there are already a few which need more then small patches.

@parisk parisk mentioned this issue Aug 26, 2016
20 tasks
@parisk
Copy link
Contributor

parisk commented Aug 26, 2016

Interesting discussion happens also at codemirror/codemirror5#4200.

@131
Copy link

131 commented Aug 26, 2016

Yep, i dont use defaut export too.
Also, code modularisation is what all this is about, i'v renamed my PR "ES6 remap" to "ES6 modularisation" also 😄

@adrianheine
Copy link

We use rollup for building, and plan to use buble for ES6 transpiling.

@parisk
Copy link
Contributor

parisk commented Aug 27, 2016

Hi @adrianheine, I saw that your PR in CodeMirror 👍 . Great job.

Rollup worked amazingly well, except for the testing part. I couldn't find a way to make it work with Mocha and I didn't want to use different transpilers (rollup and babel) for output and tests.

Any ideas?

parisk referenced this issue Aug 29, 2016
Introduce Babel as the build system, convert src/xterm.js to ES2015 and export EventEmitter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants