search

dperrymorrow / electron-css-reload

force reload CSS on change in your electron app
MIT License
4 stars 4 forks source link

'persistent' opt typo corrected, also checking for isWin to nix leading "/" #1

Closed liambrownweb closed 7 years ago

liambrownweb commented 7 years ago

Hi, this is my first PR ever, so I'm apologizing ahead of time in case it breaks unwritten protocol (or written protocol that I haven't seen; I'm all ears in that case). In trying to implement a CSS change watch for my Electron project, I noticed that updates only happened once. I found two apparent bugs. One was a simple typo (fs.watchFile() expects a key "persistent" in the options object, and reload.js was spelling it "persistant"), and the other was a bit more complex. I'm running this in a Win32 environment because of client requirements, and _nodePath() prepends an unnecessary "/" on the file path. So the result of _nodePath("C:/big_dir/small_dir/file.css") is "/C:/big_dir/small_dir/file.css". By the time fs is setting the watch on the file, it's looking for a file at "C:/C:/big_dir/small_dir/file.css", which of course means that the reload function is never triggered. I inserted a test for "win" in the platform string, which if found, causes the script to trim off that leading character from the _nodePath output. No idea whether this will work elsewhere, so external verification is welcome.

dperrymorrow commented 7 years ago

hey thanks for the typo fix, I do not use windows so I don't really have a way to test this, but I think that using path.normalize might be a better way to fix up the path for windows.

If you did that the isWin check would no longer be needed. If you change your pull to use path.normalize ill merge this, thanks again.

liambrownweb commented 7 years ago

Yikes, I really let this sit too long. Awfully sorry for that. I've investigated path.normalize as an option, and it didn't work as hoped. Based on present information, I think you were right to proceed with the merge. I'll keep an eye on this and provide refinements if any appear necessary.

Interesting that Electron's Node prepends Windows paths with a slash. I wonder if it would be better to test for the slash as well as the platform, in case the behavior changes. Since I have access to a Windows environment for the foreseeable future, I'll keep this on my watch-list and try to help ensure that nothing breaks.

On Tue, Feb 21, 2017 at 2:32 PM, David Morrow notifications@github.com wrote:

Merged #1 https://github.com/dperrymorrow/electron-css-reload/pull/1.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dperrymorrow/electron-css-reload/pull/1#event-971518457, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4WcvZTUkospgEfK3Ggft0ns1kFkDNAks5re1gFgaJpZM4MA2Hf .

dperrymorrow commented 7 years ago

thanks yeah any help with windows is much appreciated as i am only ever on a mac