Compare paths in a case-insensitive manner #1381
Conversation
gavande1
left a comment
gavande1
left a comment
There was a problem hiding this comment.
It works as expected. LGTM 👍
|
@fredrikekelund sure, looks good. What do you think about updating the older code to use the unified approach? |
|
Makes sense 👍 I'll take a stab at that in this PR before landing. |
fredrikekelund
changed the title
|
I've updated the PR to also cover |
wojtekn
left a comment
wojtekn
left a comment
There was a problem hiding this comment.
The code change looks good, and it works as expected.
| ); | ||
| } | ||
|
|
||
| // Compare paths in a case-insensitive manner. `fs.Stats.dev` signifies the device ID, and |
There was a problem hiding this comment.
Thanks for adding the explanation.
nightnei
left a comment
nightnei
left a comment
There was a problem hiding this comment.
LGTM, just left few comments
| } ); | ||
| } | ||
|
|
||
| export function comparePaths( event: IpcMainInvokeEvent, path1: string, path2: string ) { |
There was a problem hiding this comment.
I am thinking - should we go with the same naming for internal and for IPC?
Or, mauyb e as option woudl be better to come up with some rule to follow inside our team, e.g. - if you want to expose some function, use the same name but with some prefix or something like that.
WDYT?
| export function comparePaths( event: IpcMainInvokeEvent, path1: string, path2: string ) { | |
| export function arePathsEqual( event: IpcMainInvokeEvent, path1: string, path2: string ) { |
There was a problem hiding this comment.
I agree they would ideally use the same name, this was just my lazy choice to work around the problem of name collisions in ipc-handlers.ts. I hit "merge" just before you submitted your review, so I would just leave this as is.
There was a problem hiding this comment.
Sure, let's leave it as is, I just wanted to raise this discussion for future cases
| // the current file system's case sensitivity. | ||
| export function arePathsEqual( path1: string, path2: string ) { | ||
| try { | ||
| const stats1 = fs.statSync( path.resolve( path1 ) ); |
There was a problem hiding this comment.
I noticed that we are friquently use sync options of functions. In this case I think it's not a problem, but I am thinking - should we have some rule to try to always use async versions, to avoid any potential cases to block event loop?
There was a problem hiding this comment.
fs.exists is deprecated, but fs.existsSync is not, so that's why we'd use the sync version of that particular function. As for fs.statSync, we could technically use the async version. Given that there's only a single client to this code (this isn't a web server handling multiple clients in parallel), I think we can be more liberal with using sync functions than we would otherwise.
Still, it would be very simple to change these calls to their async counterparts, so I agree it would have been better if I had done that. This code lived in just the CLI to begin with, where the demand for asynchrony is even lower, and I think I just overlooked that part when moving it to ./common
There was a problem hiding this comment.
Yeah, totally agree about not having any issues with CLI.
It was more general discussion - to have an agreement inside our team to try to always use async versions, to have consistency and avoid potential cases when some sync version can be unintentionally used inside main app.
Btw, interesting thing - https://stackoverflow.com/questions/31799274/node-js-fs-exists-will-be-deprecated-what-to-use-instead
The [io.js docs](https://iojs.org/api/fs.html#fs_fs_exists_path_callback#fs_fs_exists_path_callback) mention the use of fs.stat() or fs.access() in place of fs.exists().
There was a problem hiding this comment.
some sync version can be unintentionally used inside main app
Since the node.js APIs aren't available in the renderer process, I don't think it's technically possible to block the renderer by using those functions.
I think we can be pragmatic about using sync functions, but I agree that we should always default to the async versions.
Related issues
Proposed Changes
Our existing way of looking up Studio sites produces unexpected results on case-insensitive file systems if users have renamed any of the directories in the path to use a different case. Here's a concrete example:
/Users/fredrik/Studio/my-wordpress-website.Studiofolder toSTUDIO.process.cwd()returns/Users/fredrik/STUDIO/my-wordpress-websiteand my Studio config file contains/Users/fredrik/Studio/my-wordpress-website.This PR fixes the issue by using
fs.statand comparingfs.Stats.devandfs.Stats.inobetween the CLI path and the config file path.Moreover, I've updated
src/hooks/use-add-site.ts. This hook would previously rely on comparing paths by always converting them to lowercase. We now use the same logic there as we do in the CLI.Testing Instructions
Test the CLI:
npm run cli:build.node /dist/cli/main.js preview list --path ~/Studio/my-wordpress-website(update the--pathoption to point to a Studio website).node /dist/cli/main.js preview list --path ~/STUDIO/my-wordpress-website(use the same--pathoption, but change the case in some part of it).Test the app:
npm start.Add sitebutton.Pre-merge Checklist