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

serve-live-assets-autorefresh strategy #37

Merged
merged 7 commits into from
Jul 13, 2015

Conversation

antonyshchenko
Copy link
Contributor

Implementation of ideas from #34

@magnars
Copy link
Owner

magnars commented Apr 27, 2015

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

@antonyshchenko
Copy link
Contributor Author

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
Copy link
Owner

magnars commented Apr 27, 2015

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
#37 (comment).

@antonyshchenko
Copy link
Contributor Author

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
#37 (comment).


Reply to this email directly or view it on GitHub
#37 (comment).

Best Regards,
Anton Onyshchenko

@magnars
Copy link
Owner

magnars commented Apr 27, 2015

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
#37 (comment).


Reply to this email directly or view it on GitHub
#37 (comment).

Best Regards,
Anton Onyshchenko


Reply to this email directly or view it on GitHub
#37 (comment).

@antonyshchenko
Copy link
Contributor Author

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

@magnars
Copy link
Owner

magnars commented May 23, 2015

This looks pretty great, mate. Well done! 👍 😄

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 added a commit to antonyshchenko/optimus that referenced this pull request May 29, 2015
@antonyshchenko
Copy link
Contributor Author

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 antonyshchenko@7d7f4fb

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
Copy link
Owner

magnars commented Jul 10, 2015

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
Copy link
Contributor Author

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

@magnars
Copy link
Owner

magnars commented Jul 13, 2015

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

magnars added a commit that referenced this pull request Jul 13, 2015
serve-live-assets-autorefresh strategy
@magnars magnars merged commit 6c6b2b4 into magnars:master Jul 13, 2015
@magnars
Copy link
Owner

magnars commented Jul 13, 2015

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

@antonyshchenko
Copy link
Contributor Author

Nice! Thank you :)

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.

2 participants