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

Integrate Browsersync #681

Merged
merged 17 commits into from
Feb 9, 2016
Merged

Integrate Browsersync #681

merged 17 commits into from
Feb 9, 2016

Conversation

jescalan
Copy link
Owner

Ok guys here it is, the big integration.

This still needs a bit more polish, but is working fully. Server works, reloads work, no bugs or edge cases I have been able to find. Would love to have some people pull this down and run it with their projects!

This PR depends on #680, and in fact both require each other in order to have a functional roots (a bug in infestor is causing a problem with images). However, I wanted to keep the PRs separate for clarity.

@dbox
Copy link

dbox commented Sep 29, 2015

🎉 What's the best way to test a site with a non-published version of roots?

@jescalan
Copy link
Owner Author

  1. Clone roots and checkout this branch
  2. Run npm link
  3. Test away!
  4. When you are done and want to switch back to 3.1.x (if you do), just npm i roots -g and you'll be back to normal

@dbox
Copy link

dbox commented Sep 29, 2015

Great. Thanks. Do I need to merge deps and this branch first?

@jescalan
Copy link
Owner Author

Nope, just check out this branch and you're set!

@dbox
Copy link

dbox commented Sep 29, 2015

  • Image issue fixed
  • font issue fixed
  • browsersync working

@dbox
Copy link

dbox commented Sep 29, 2015

2 other repos (my original image demo + axis-www) give this error:

Potentially unhandled rejection [1] TypeError: Cannot read property 'clean_urls' of undefined
  at Server.start (/Users/danielbox/sites/dev/roots/lib/local_server.coffee:46:12)
  at dispatch (/Users/danielbox/sites/dev/roots/node_modules/when/node.js:70:15)
  at callAndResolve (/Users/danielbox/sites/dev/roots/node_modules/when/lib/apply.js:30:12)
  at callAndResolveNext (/Users/danielbox/sites/dev/roots/node_modules/when/lib/apply.js:40:4)
  at tryCatchReject3 (/Users/danielbox/sites/dev/roots/node_modules/when/lib/makePromise.js:856:7)
  at runContinuation3 (/Users/danielbox/sites/dev/roots/node_modules/when/lib/makePromise.js:814:4)
  at Fulfilled.fold (/Users/danielbox/sites/dev/roots/node_modules/when/lib/makePromise.js:588:4)
  at callAndResolve (/Users/danielbox/sites/dev/roots/node_modules/when/lib/apply.js:34:12)
  at apply (/Users/danielbox/sites/dev/roots/node_modules/when/lib/apply.js:23:4)
  at Object.call (/Users/danielbox/sites/dev/roots/node_modules/when/node.js:107:10)
  at Compile.<anonymous> (/Users/danielbox/sites/dev/roots/lib/cli/watch.coffee:44:12)
  at tryCatchReject (/Users/danielbox/sites/dev/roots/node_modules/when/lib/makePromise.js:845:30)
  at runContinuation1 (/Users/danielbox/sites/dev/roots/node_modules/when/lib/makePromise.js:804:4)
  at Fulfilled.when (/Users/danielbox/sites/dev/roots/node_modules/when/lib/makePromise.js:592:4)
  at Pending.run (/Users/danielbox/sites/dev/roots/node_modules/when/lib/makePromise.js:483:13)
  at Scheduler._drain (/Users/danielbox/sites/dev/roots/node_modules/when/lib/Scheduler.js:62:19)
  at Scheduler.drain (/Users/danielbox/sites/dev/roots/node_modules/when/lib/Scheduler.js:27:9)
  at process._tickCallback (node.js:442:13)

which shuts down the watcher

@jescalan
Copy link
Owner Author

Ah great that kind of bug is just what I was after. Nice catch. Fixed in the latest commit, pull down and try again!

@dbox
Copy link

dbox commented Sep 29, 2015

giphy-1

Working on my end now!

@jescalan
Copy link
Owner Author

Dope. Thanks for the help here. I'm going to try to smooth out a couple things and run some tests on a couple larger carrot projects. When these are ready, I'll ship this guy and we can get back to making real progress with this codebase, finally.

@dbox
Copy link

dbox commented Sep 29, 2015

Thanks as always. I ❤️ roots

@SevereOverfl0w
Copy link
Contributor

Build failing due to manual call to server.close in a test: https://github.com/jenius/roots/blob/browsersync/test/cli.coffee#L187 should it be calling obj.stop() instead?

Also, the stop method should call cb() after @bs.exit

@bs.notify('<div id="roots-load-container"><div id="roots-compile-loader">
<div id="l1"></div><div id="l2"></div><div id="l3"></div><div id="l4"></div>
<div id="l5"></div><div id="l6"></div><div id="l7"></div><div id="l8"></div>
</div></div>')
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a formatting preference thing, but any reason you're not using CoffeeScript's awesome multiline strings for embedded HTML snippets?

@bs.notify """
  <div id="roots-load-container">
    <div id="roots-compile-loader">
      <div id="l1"></div>
      <div id="l2"></div>
      <div id="l3"></div>
      <div id="l4"></div>
      <div id="l5"></div>
      <div id="l6"></div>
      <div id="l7"></div>
      <div id="l8"></div>
    </div>
  </div>
"""

@zspecza
Copy link
Contributor

zspecza commented Oct 14, 2015

I have a few ideas:

Replacing Chokidar entirely

If the browser-sync instance is decoupled from the Server class, chokidar in /api/watch.coffee can be replaced with the BS instance's various watch/stream methods, making it one less dependency.

Enabling live-injection of compiled assets

Following the above change, observing the compiled output and calling bs.reload() on specific files will live-inject the changes into the browser where possible. This gives the developer faster visual feedback.

  • This is possible with CSS. bs.reload('*.css')
  • I believe this is also possible with image assets. bs.reload(['*.jpg', '*.png', ...])
  • It's possible to do this with HTML changes too, with a plugin

Selective Compile / Incremental Builds

I'm not 100% certain on Roots' internals, but on Windows when I change a file Roots seems to compile every file, rather than just the one being changed. Between the 3-7s time it takes to finish compiling on a busy machine running git shell through a console emulator alongside a running Node server and the time it takes to reload the page (often not reloading at all), which is a common workflow for Node development on Windows, this can become rather irksome.

I think browser-sync offers a more stable ground to build on regarding cross-platform compatibility when it comes to incremental builds/selective compiles (I hope one of those two terms is the correct one). There is a challenge here though, in that it might be unwise to watch for all the different file extension combinations a Roots project could possibly have - perhaps this can be found out from the sprout template being used, or perhaps the existence of transpilers in the package.json? Just a thought.

@jescalan
Copy link
Owner Author

  1. The problem here is just the dependency tree, not chokidar. Unfortunately browsersync has no solution to the dependency tree issue.
  2. Possible but a janky as hell solution. Would need a mess of language-specific rules and overrides and it's just not something I want to maintain.
  3. The problem again is the dependency tree. If you update a layout file, what does it build? If you update a partial, how does it determine what uses that partial? It doesn't so selective compile actually leads to more errors.

@molovo
Copy link

molovo commented Dec 9, 2015

Nice job. Certainly fixed the issues I was having with images.

I did find one slight niggle though, which was introduced somewhere in this PR. When running roots watch --no-open the browser window still launches. Setting open_browser: false in app.coffee fails as well. Is this something that was dropped deliberately, or something that browsersync handles for us now. (If so, how?)

@zspecza
Copy link
Contributor

zspecza commented Dec 10, 2015

@molovo my guess is browser-sync is opening the browser window.

It seems passing open: false to browser-sync is the way to stop this behavior.

This change allows you to pass options to browser-sync in app.coffee using the browser key.

Put this together and:

app.coffee

module.exports =
  ...
  browser:
    open: false

Perhaps it might be a good idea to set this automatically if the respective root's "no open" config settings are present...

/cc @Jenius

@SevereOverfl0w
Copy link
Contributor

Well, we either change the docs or roots. I think the --no-open should stick, but should just set

browser:
  open: false

@SevereOverfl0w
Copy link
Contributor

#693 Now fixed

@FaizShah
Copy link

Any update on the dependency graph issue?

@jescalan
Copy link
Owner Author

@FaizShah nope, there will not be one until a new major version of roots comes out. getting a consistent dependency graph with a large number of flexible compiled languages is immensely difficult. Next version is in the works though and will guaranteed have incremental builds with full dependency trees, and slightly restricted available languages. More info on this as things stabilize, which they are not even close to now.

@FaizShah
Copy link

@Jenius Great, thanks for the info. Good luck with v4!

@jescalan
Copy link
Owner Author

Ok finally this looks ready to go imho. All dependencies up to date, browsersync working nicely 😀

jescalan pushed a commit that referenced this pull request Feb 9, 2016
@jescalan jescalan merged commit e562eb6 into master Feb 9, 2016
@jescalan jescalan deleted the browsersync branch February 9, 2016 18:08
@zspecza
Copy link
Contributor

zspecza commented Feb 9, 2016

🎊

@dbox
Copy link

dbox commented Feb 9, 2016

whooot!

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.

None yet

6 participants