Skip to content

Introduce build system based on Gulp #435

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.

Sign up for GitHub

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

Jump to bottom
Merged
parisk merged 2 commits into from
Jan 15, 2017
Merged

Introduce build system based on Gulp #435

parisk merged 2 commits into from
Jan 15, 2017

Conversation

@parisk
Copy link
Contributor

@parisk parisk commented Dec 31, 2016

This PR closes #359.

Details

  • Add Gulp and new dependencies to package.json
  • Add gulpfile.js with two tasks:
    • tsc: For building TypeScript sources
    • build: For building and bundling JavaScript modules in a monolith
  • Clean up Dockerfile, since cpio is not needed any more
  • Clean up not needed dependencies from package.json
  • Remove bin/build
  • Update bin/release to use npm run build instead of ./bin/build

@parisk parisk self-assigned this Dec 31, 2016
@parisk parisk requested review from Tyriar and akalipetis December 31, 2016 10:00
Tyriar
Tyriar reviewed Dec 31, 2016
View reviewed changes
gulpfile.js
const gulp = require('gulp');
const merge = require('merge-stream');
const source = require('vinyl-source-stream');
const sourcemaps = require('gulp-sourcemaps');
Copy link
Member

@Tyriar Tyriar Dec 31, 2016

Choose a reason for hiding this comment

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

Sourcemaps don't appear to be working on the demo.

image

sorcery was being used because the browserify version needs to resolve dist/xterm.js -> lib/xterm.js > src/xterm.js.

Copy link
Contributor Author

@parisk parisk Jan 9, 2017

Choose a reason for hiding this comment

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

Hmm, I'll take a look. gulp-sourcemaps was supposed to do this when using loadMaps.

Copy link
Member

@Tyriar Tyriar Jan 9, 2017

Choose a reason for hiding this comment

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

If this could be fixed before this was merged that would be great as I use dev tools pretty heavily for xterm.js dev.

Tyriar
Tyriar requested changes Dec 31, 2016
View reviewed changes
package.json
"test": "mocha --recursive ./lib",
"build:docs": "jsdoc -c jsdoc.json",
"build": "./bin/build",
"build": "gulp build",
Copy link
Member

@Tyriar Tyriar Dec 31, 2016

Choose a reason for hiding this comment

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

Unrelated but can we merge npm run dev into npm start? I was confused as to why watching wasn't happening on npm start. I don't think there's any point to not watch on the dev version?

Copy link
Contributor Author

@parisk parisk Jan 9, 2017

Choose a reason for hiding this comment

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

Let's leave this for another PR to keep this as much in context as possible.

gulpfile.js Outdated
.pipe(source('xterm.js'))
.pipe(buffer())
.pipe(sourcemaps.init({
loadMaps: true
Copy link
Member

@Tyriar Tyriar Dec 31, 2016

Choose a reason for hiding this comment

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

Might be more readable on one line:

.pipe(sourcemaps.init({ loadMaps: true }))

package.json
"fs-extra": "^1.0.0",
"glob": "^7.0.5",
"gulp": "^3.9.1",
"gulp-cli": "^1.2.2",
Copy link
Member

@Tyriar Tyriar Dec 31, 2016

Choose a reason for hiding this comment

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

I don't think you install this in the project, only globally.

Copy link
Contributor Author

@parisk parisk Jan 9, 2017

Choose a reason for hiding this comment

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

When used in an package.json script it works. Npm adds ./node_modules/.bin in PATH when running via npm run ....

@Tyriar Tyriar mentioned this pull request Jan 1, 2017
Merged
@parisk parisk added the work-in-progress Do not merge label Jan 9, 2017
@parisk
Copy link
Contributor Author

parisk commented Jan 9, 2017

Status update based on @Tyriar's comment #435 (comment) on broken source maps.

It seems that gulp-typescript with gulp-sourcemaps produce different source maps, compared to the ones produced by tsc.

The most important change I found out is that tsc does not inline source content into the source maps, while gulp-sourcemaps does.

Other than that it seems that the structure is different as well and a wild guess is that the new source maps have messed up sourceRoot. I am taking a deeper look and will post an update.

@parisk parisk force-pushed the issue-#359-gulp branch 2 times, most recently from 4eab7a9 to 0cc5be7 Compare January 11, 2017 02:37
@parisk
Copy link
Contributor Author

parisk commented Jan 11, 2017

@Tyriar everything should be fine now (except for the tests on Travis, which I have to fix).

Can you please give it a try and also run the tests and let me know if npm run test crashes on your environment as well? Runs fine on SourceLair, but crashes on Travis.

@Tyriar
Copy link
Member

Tyriar commented Jan 11, 2017

I see the same issue, must be a Linux problem with the paths (it should contain xterm.js):

Error: ENOENT: no such file or directory, open '/home/daniel/dev/source
lair/lib/CompositionHelper.js.map'

@Tyriar
Copy link
Member

Tyriar commented Jan 11, 2017 •
edited
Loading

It's not working because the initial sourcemaps are wrong:

{"version":3,"sources":["/src/xterm.js"],"

Should be:

{"version":3,"sources":["../src/xterm.js"],"

Or (what it is before this PR):

{"version":3,"file":"xterm.js","sourceRoot":"","sources":["../src/xterm.js"]

@parisk
Fix #359 - Introduce build system based on Gulp
081fe3f 
- Add Gulp and new dependencies to `package.json`
- Add `gulpfile.js` with four tasks:
    - `tsc`: For building TypeScript sources
    - `bundle`: For bundling JavaScript modules in a monolith
    - `sorcery`: For resolving the source map chains back to the original TypeScript files
    - `build` (`default`): Runs the whole `tsc` → `bundle` → `sorcery` chain
- Clean up `Dockerfile`, since `cpio` is not needed any more
- Clean up not needed dependencies from `package.json`
- Remove `bin/build`
- Update `bin/release` to use `npm run build` instead of `./bin/build`
@parisk parisk removed the work-in-progress Do not merge label Jan 14, 2017
@parisk
Copy link
Contributor Author

parisk commented Jan 14, 2017

@Tyriar this should be 👌 for review. The error was caused by gulp-sourcemaps/gulp-sourcemaps@fb4027a.

Tyriar
Tyriar approved these changes Jan 15, 2017
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

It works 😄

@parisk parisk merged commit 6309752 into master Jan 15, 2017
@Tyriar Tyriar deleted the issue-#359-gulp branch January 15, 2017 10:10
@Tyriar Tyriar modified the milestone: 2.3.0 Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tyriar Tyriar Tyriar approved these changes

@akalipetis akalipetis Awaiting requested review from akalipetis

Assignees

@parisk parisk

Labels

None yet

Projects

None yet

Milestone

2.3.0

Development

Successfully merging this pull request may close these issues.

Introduce task runner for builds

3 participants

@parisk @Tyriar