broccolijs / broccoli

Browser compilation library – an asset pipeline for applications that run in the browser
https://broccoli.build
MIT License
3.33k stars 216 forks source link

Cli option --watch #342

Closed chrmod closed 6 years ago

chrmod commented 6 years ago

To be merged after: https://github.com/broccolijs/broccoli/pull/341 Implements another part of: https://github.com/broccolijs/broccoli/issues/331 Closes: https://github.com/broccolijs/broccoli/issues/184

Notable changes:

Questions:

chrmod commented 6 years ago

@stefanpenner @rwjblue please review as some solutions here may be questionable.

chrmod commented 6 years ago

Some of the tests does not close the server and hang. Need to detect which one it is but as it happens only in CI I will be pushing commits to this PR to detect problematic test. Please do not merge even if PR is green.

chrmod commented 6 years ago

I wanted to cover the case on which server is closed on user input (SIGTERM)B On current master I cannot close server after it serves content for the first time, had to add: https://github.com/broccolijs/broccoli/pull/342/files#diff-c945a46d13b34fcaff544d966cffcabaR47

Have created a test, but it is not working correctly in CI. Maybe someone can suggest how to approach the problem.

Problematic test:

    context('on terminated watcher', function() {
      it('by SIGTERM exits without error', function(done) {
        const closingPromise = cli(['node', 'broccoli', 'serve']);
        setTimeout(() => {
          process.kill(process.pid, 'SIGTERM');
        }, 200);
        process.once('SIGTERM', () => {
          closingPromise
            .then(() => {
              chai.expect(exitStub).to.be.calledWith(0);
              done();
            })
            .catch(done);
        });
      });
    });

The PR is ready for merge just after https://github.com/broccolijs/broccoli/pull/341 and rebasing.

stefanpenner commented 6 years ago

LGTM, sorry for the delay was AFK for a-few days.

@chrmod this looks good to ship, can you confirm? (I'll merge once you confirm)

chrmod commented 6 years ago

@stefanpenner it is fine to merge, thanks!

danielo515 commented 6 years ago

How can I make use of this ? There is no reference on the cli help and any option I tried ends on a unknown option --watch