magnars / optimus

A Ring middleware for frontend performance optimization.
364 stars 23 forks source link

serve-live-assets-autorefresh strategy #37

Closed antonyshchenko closed 9 years ago

antonyshchenko commented 9 years ago

Implementation of ideas from #34

magnars commented 9 years ago

Seems like this fails on JDK 6 because of nio. That would make this a breaking change. Any thoughts?

antonyshchenko commented 9 years ago

I did a quick research and it looks like we will have to use polling for this in JDK 6 (existing clojure solutions don't support JDK 6).

Now we have 3 options:

  1. Use some java library for polled filesystem watching.
  2. Write a small clojure implementation for this.
  3. Make it a breaking change and don't support JDK 6 anymore :)

What would you prefer?

magnars commented 9 years ago

I think I would prefer another solution, if possible:

Possible?

On Mon, Apr 27, 2015 at 9:57 AM Anton Onyshchenko notifications@github.com wrote:

I did a quick research and it looks like we will have to use polling for this in JDK 6 (existing clojure solutions don't support JDK 6).

Now we have 3 options:

  1. Use some java library for polled filesystem watching.
  2. Write a small clojure implementation for this.
  3. Make it a breaking change and don't support JDK 6 anymore :)

What would you prefer?

— Reply to this email directly or view it on GitHub https://github.com/magnars/optimus/pull/37#issuecomment-96544781.

antonyshchenko commented 9 years ago

Good idea. I will implement it. Should I also update the documentation in order to make users aware of this feature?

On Mon, Apr 27, 2015 at 11:42 AM, Magnar Sveen notifications@github.com wrote:

I think I would prefer another solution, if possible:

  • throw an exception when trying to use the new strategy on jdk 6.
  • don't run these tests when on jdk 6.

Possible?

On Mon, Apr 27, 2015 at 9:57 AM Anton Onyshchenko < notifications@github.com> wrote:

I did a quick research and it looks like we will have to use polling for this in JDK 6 (existing clojure solutions don't support JDK 6).

Now we have 3 options:

  1. Use some java library for polled filesystem watching.
  2. Write a small clojure implementation for this.
  3. Make it a breaking change and don't support JDK 6 anymore :)

What would you prefer?

— Reply to this email directly or view it on GitHub https://github.com/magnars/optimus/pull/37#issuecomment-96544781.

— Reply to this email directly or view it on GitHub https://github.com/magnars/optimus/pull/37#issuecomment-96585601.

Best Regards, Anton Onyshchenko

magnars commented 9 years ago

That would be great!

On Mon, Apr 27, 2015 at 11:58 AM Anton Onyshchenko notifications@github.com wrote:

Good idea. I will implement it. Should I also update the documentation in order to make users aware of this feature?

On Mon, Apr 27, 2015 at 11:42 AM, Magnar Sveen notifications@github.com wrote:

I think I would prefer another solution, if possible:

  • throw an exception when trying to use the new strategy on jdk 6.
  • don't run these tests when on jdk 6.

Possible?

On Mon, Apr 27, 2015 at 9:57 AM Anton Onyshchenko < notifications@github.com> wrote:

I did a quick research and it looks like we will have to use polling for this in JDK 6 (existing clojure solutions don't support JDK 6).

Now we have 3 options:

  1. Use some java library for polled filesystem watching.
  2. Write a small clojure implementation for this.
  3. Make it a breaking change and don't support JDK 6 anymore :)

What would you prefer?

— Reply to this email directly or view it on GitHub https://github.com/magnars/optimus/pull/37#issuecomment-96544781.

— Reply to this email directly or view it on GitHub https://github.com/magnars/optimus/pull/37#issuecomment-96585601.

Best Regards, Anton Onyshchenko

— Reply to this email directly or view it on GitHub https://github.com/magnars/optimus/pull/37#issuecomment-96588824.

antonyshchenko commented 9 years ago

@magnars, I've finally fixed tests and updated documentation. Please review it and merge if you thing it's good enough :)

magnars commented 9 years ago

This looks pretty great, mate. Well done! :+1: :smile:

I'm happy to merge this, but one question first: Have you tried writing the tests without with-redefs on the watch-dir function? When you're using test-with-files, there are real files on disk, so maybe it would be better without the mocking?

antonyshchenko commented 9 years ago

Actually I have a problem with trying to avoid mock here - watch-dir function does not work properly in tests for some reason. Please take a look at https://github.com/env0der/optimus/commit/7d7f4fb963e315afb8244c2c9aa24f751d0fc8f2

The dir watcher defined in a test is notified only when you change the file via some other process (text editor, or clojure repl). I have no idea why this happens. Maybe you can help?

magnars commented 9 years ago

I finally found some time to look at this. After a frustrating time, I thought to run dirwatch's own tests, which also do not run on my machine. I remember implementing a file-watcher for node several years ago, and Mac OSX was certainly the worst of the bunch when it came to triggering change events in the file system.

I think we'll have to resort to mocks in this case.

antonyshchenko commented 9 years ago

Then you can merge my pull-request. I've reverted the last commit, so now it relies on mocking watch-dir function again.

magnars commented 9 years ago

Well done, Sir. Thanks for sticking with it through all this.

magnars commented 9 years ago

0.18.0 released, and your name added to the list of contributors. Thanks again! :)

antonyshchenko commented 9 years ago

Nice! Thank you :)