Compare paths in a case-insensitive manner

[fredrikekelund]

Related issues

• Fixes STU-487

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:

• I'm using macOS's default file system.
• I have a Studio site in /Users/fredrik/Studio/my-wordpress-website.
• I rename the Studio folder to STUDIO.
• Studio continues to function normally, since the path still resolves to an actual location on disk.
• However, the CLI no longer works, since process.cwd() returns /Users/fredrik/STUDIO/my-wordpress-website and my Studio config file contains /Users/fredrik/Studio/my-wordpress-website.

This PR fixes the issue by using fs.stat and comparing fs.Stats.dev and fs.Stats.ino between 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:

1. Run npm run cli:build.
2. Run node /dist/cli/main.js preview list --path ~/Studio/my-wordpress-website (update the --path option to point to a Studio website).
3. Run node /dist/cli/main.js preview list --path ~/STUDIO/my-wordpress-website (use the same --path option, but change the case in some part of it).

Test the app:

1. Run npm start.
2. Click the Add site button.
3. Enter the name of a site that's already added to Studio (and that resides in Studio's default site directory).
4. Ensure that an error is displayed.
5. Change the site name to something else.
6. Manually specify a site path. Choose a site path that's already occupied by an existing Studio site.
7. Ensure that an error is displayed.

Pre-merge Checklist

• Have you checked for TypeScript, React or other console errors?

[gavande1]

It works as expected. LGTM 👍

[fredrikekelund]

@wojtekn, just confirming that you are also OK with this approach, given our discussion in #1377?

[wojtekn]

@fredrikekelund sure, looks good. What do you think about updating the older code to use the unified approach?

[fredrikekelund]

Makes sense 👍 I'll take a stab at that in this PR before landing.

[fredrikekelund]

I've updated the PR to also cover useAddSite. Would appreciate another review 🙏

[wojtekn]

The code change looks good, and it works as expected.

[wojtekn]

Thanks for adding the explanation.

[nightnei]

LGTM, just left few comments

[nightnei]

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?

Suggested change

	export function comparePaths( event: IpcMainInvokeEvent, path1: string, path2: string ) {
	export function arePathsEqual( event: IpcMainInvokeEvent, path1: string, path2: string ) {

[fredrikekelund]

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.

[nightnei]

Sure, let's leave it as is, I just wanted to raise this discussion for future cases

[nightnei]

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?

[fredrikekelund]

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

[nightnei]

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().

[fredrikekelund]

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.