halohalospecial / atom-elmjutsu

A bag of tricks for developing with Elm. (Atom package)
https://atom.io/packages/elmjutsu
MIT License
192 stars 24 forks source link

Fixes #107 Verify filePath is a file in sendFileContentsChanged #108

Closed AsaAyers closed 6 years ago

AsaAyers commented 6 years ago

The error in #107 was triggered because my project has a symlink to a folder. It looks like the symlink gets picked up by the watcher, but because it points to a folder it isn't compatible with fs.readFileSync.

https://github.com/halohalospecial/atom-elmjutsu/blob/33d6ccce89057e5cd4f648f268b30cd6c9c8c313/lib/core.js#L363-L378

halohalospecial commented 6 years ago

Thanks for this, @AsaAyers! Do you think we should check if it's a symlink, then check if the linked thing is a file?

AsaAyers commented 6 years ago

Based on this test, it looks like .isFile() acts based on the destination and not the symlink itself. And if the symlink does point to a file, .readFileSync() works.

$ mkdir /tmp/foo
$ cd /tmp/foo
$ ln -s /opt
$ ln -s /etc/hosts

$ node  --print 'require("fs").statSync("./opt").isFile()'
false
$ node  --print 'require("fs").statSync("./hosts").isFile()'
true
$ node  --print 'require("fs").readFileSync("./hosts").toString()'
127.0.0.1   localhost
...

When I was debugging and logged filePath, I saw many files that weren't .elm. Should non-elm files get filtered out somewhere? I didn't make that change because it's not clear to me what impact that would have.

halohalospecial commented 6 years ago

@AsaAyers, I think you're right. There's a similar check for .elm here: https://github.com/AsaAyers/atom-elmjutsu/blob/86890822b8e700cbd3e8e72768b1eade635163e0/lib/indexing.js#L184

halohalospecial commented 6 years ago

@AsaAyers, sorry I forgot to publish the fix!

Can you check v7.2.3? Thanks again!