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

Fixes for watch mode. #1851

Merged
merged 6 commits into from
Jan 30, 2015
Merged

Fixes for watch mode. #1851

merged 6 commits into from
Jan 30, 2015

Conversation

pavel
Copy link
Contributor

@pavel pavel commented Jan 28, 2015

Problems that are fixed in the request:

  1. watch father file of includes #1086
  2. problem, that when watching a folder jade-cli was not watching subtrees.

Solutions provided:

  1. fetch dependencies from jade.compile, and watch'em.
  2. watch every file that is a target for render.

Improvement:

  1. Unwatching a file, if the file was moved or removed.

@ForbesLindesay
Copy link
Member

Travis is failing :(

@pavel
Copy link
Contributor Author

pavel commented Jan 29, 2015

But why???
1257.1 Some timeout issues, when running in non-watch mode.
1257.2 Timeout when running "jade --help".

@pavel
Copy link
Contributor Author

pavel commented Jan 29, 2015

I ran npm run coveralls and it succeded. Well partially, in the end it failed, because it was not able to write report or smthn.

files.forEach(renderFile);
// unwatch what's watched on exit
process.on("exit", function () {
watchList.forEach(fs.unwatchFile);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to do this. When node exits it will just stop watching the files at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me verify this.

@ForbesLindesay
Copy link
Member

I've added a few comments. Other than those, it looks pretty good.

To fix the timeouts, I would suggest going through https://github.com/jadejs/jade/blob/master/test/command-line.js and everywhere we have something like:

  if (isIstanbul) {
    this.timeout(8000);
    this.slow(6000);
  } else {
    this.slow(250);
  }

make it something like

  if (isIstanbul) {
    this.timeout(60000);
    this.slow(6000);
  } else {
    this.timeout(30000);
    this.slow(3000);
  }

We want timeouts that are way longer than we expect them to take, rather than having them on the brink of failing every time. Sorry for assuming your change caused this initially.

@pavel
Copy link
Contributor Author

pavel commented Jan 29, 2015

Sure, I'll increase the timeouts in this pull-request. Don't be sorry, you do what you gotta do.

@pavel
Copy link
Contributor Author

pavel commented Jan 29, 2015

I've left normalize, if anyone thinks that it's important to switch to resolve, then let me know.

@TimothyGu
Copy link
Member

@pavel I don't really like the test timeouts. 30 seconds is way too much for the timeouts, and 3 seconds for being slow isn't that reasonable either.

@pavel
Copy link
Contributor Author

pavel commented Jan 29, 2015

I did what @ForbesLindesay suggested. Not blaming, I'm not familiar with the test tools you use in jade, so I tend to listen to maintainers.
@TimothyGu What are your suggestions?

@TimothyGu
Copy link
Member

@pavel how about this:

istanbul non-istanbul
TO 20s 12.5s
slow 3s 2s

@pavel
Copy link
Contributor Author

pavel commented Jan 29, 2015

Sure, let me update.

@TimothyGu
Copy link
Member

Cool. Merged, thanks!

TimothyGu added a commit that referenced this pull request Jan 30, 2015
@TimothyGu TimothyGu merged commit b08209f into pugjs:master Jan 30, 2015
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