Skip to content

Conversation

@akalipetis
Copy link
Contributor

  1. Include a prestart script, which runs the build
  2. Make the start script just start the demo server
  3. Add a new dev script, which contains the nodemon logic
  4. nodemon now watches for changes in *.ts files and runs the start command

This allows for easier development, while also makes the different scripts we use simpler and self-contained.

@parisk
Copy link
Contributor

parisk commented Dec 12, 2016

Overall, this is a great addition 👍 , just two comments:

  1. Can you please change the web script to npm run dev in Procfile.dev?
  2. Should we add also a Procfile with web: npm start?

/.idea/
.env
build/
.vscode/
Copy link
Member

@Tyriar Tyriar Dec 12, 2016

Choose a reason for hiding this comment

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

Curious what you're ignoring here, did an extension or something change some workspace settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using VS code scripts while developing, which are saved in .vscode, so since Xterm.js is used in VS code, I felt that's Ok to keep this here 😇

I can definitely remove this if you believe it's not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave this here, since vscode might be creating any files, I think it's a good idea to ignore them for sure.

Non-blocking question: Should we change this to /.vscode/ like the /.idea/ above or not?

Copy link
Member

Choose a reason for hiding this comment

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

@akalipetis thanks, I was mainly just wondering if an extension was being naughty 😛

.vscode can only appear at the root of the repo so .vscode/ should be fine.

@akalipetis
Copy link
Contributor Author

Thanks @parisk and @Tyriar for your comments - Paris I have updated the Procfile*s.

I have also rebased with master.

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 .

It just needs to resolve the conflict in package.json before merging 🙂 .

1. Include a `prestart` script, which runs the build
2. Make the `start` script just start the demo server
3. Add a new `dev` script, which contains the nodemon logic
4. nodemon now watches for changes in `*.ts` files and runs the start command

Signed-off-by: Antonis Kalipetis <[email protected]>
Also, update the Procfile.dev with the updated `npm run dev` command.

Signed-off-by: Antonis Kalipetis <[email protected]>
@akalipetis akalipetis merged commit 1e0eb37 into xtermjs:master Dec 28, 2016
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants