Skip to content
This repository has been archived by the owner on May 3, 2023. It is now read-only.

fix: check file exists #30

Merged
merged 1 commit into from
Jul 10, 2019
Merged

Conversation

hampustagerud
Copy link
Contributor

I tried to use this in a lerna monorepo but it failed due to some files not being found in a iOS build folder, checking if the file exists before running statSync on it solved the issue 🙂

@m4tt72
Copy link
Collaborator

m4tt72 commented Jul 10, 2019

I need some more clarification! Why do you need to check if the file exists already? readdirSync() will always give you the files/dirs in the directory you give it, so this means they already exist!

but it failed due to some files not being found in a iOS build folder

What does Jetifier have to do with iOS?

@hampustagerud
Copy link
Contributor Author

It has nothing to do with iOS but since I am doing this in a monorepo multiple RN apps are symlinked in my node_modules folder. This script basically just traverses all the files, including those in the iOS build folder. I considered adding a blacklist option but that would involve much larger changes than this.

Xcode creates some lock-files for some reason and I can list them but when you run stat on them they are not found. Wrapping everything in a existsSync function call is not optimal but since this is a tool that utilises all cores and is only used when installing dependencies I thought it was a good-enough solution 🙂

@mikehardy
Copy link
Owner

I assume the sub-optimality is performance? If that's right, I looked into it a little bit.

You can see the timing statements in previous iterations of CI. This appears to be about 5-10% slower, but is still (in my opinion) within acceptable performance (it's sub-second for normal conditions, I think) and yes - it only executes on package install so it seems acceptable as a trade-off for correctness in monorepos

In general I will always go for bulletproof correctness over performance, though it's nice to have both if possible. I would also avoid a blacklist, I'd prefer to keep it zero configuration if possible

Maybe its possible to avoid some of the performance hit by treating what should be an exceptional case with exception handling vs checking every file.

@m4tt72
Copy link
Collaborator

m4tt72 commented Jul 10, 2019

I don't think most of the users will face the same issue, so applying this to Jetifier will definitely impact performance!
How about we just add a --check flag that will force file check for your use case, this way you can still use Jetifier if you a monorepo, without affecting everyone.

@mikehardy
Copy link
Owner

I could live with a command-line flag if @hampustagerud could - but I'm not too bothered by 5-10% performance in the default case either. I'd prefer to try handling it as an exception before adding a flag. I support a few libraries now and it's unbelievable how even the simplest configuration bits cause issues everywhere. The simpler the better with regard to usage 🙏

@hampustagerud
Copy link
Contributor Author

I agree that 5-10% is not a lot unless it was used for every build/reload. I don't think my project will be that special of a case now that RN 0.60 officially moved to androidx, maybe it will be but who knows? I can live with adding a flag but that would involve more documentation as well. Wrapping in try/catch performs similarly on my machine but I wanted to stay away from exception handling and just make sure that the file exists. By blindly traversing all files there will always be the possibility that some library builds some module that produces files that cannot be read for some reason. It is an edge case but it is something that breaks this code.

I am not too versed in the androidx world, I am only here because I had to upgrade a library and some of the others were not compatible so I am not sure if this is a solution but I changed the check to stay out of build folders:

if (filePath.indexOf('/build/') === -1) {
  // Handle the file...
}

This, naturally, improved performance instead since less files were found. I reduced my execution time with 50%. Would this be a better solution @m4tt72? 🙂 Like I said, I have not looked in to androidx that much so I don't know if what exists in the /build/ folders matter or not.

@mikehardy
Copy link
Owner

That's a good shot at improving performance but when I was originally building out the rn-androidx-demo test suite I use to check this I noticed that some libraries render java files (from something else? not sure what they were doing) and their java is in a build folder, so we can' blindly exclude on the build path

Thanks for checking the try/catch performance, I had a hunch it would involve it's own performance hit

@m4tt72 I know you are concerned with performance, as you are responsible for about 1000% of the speedups in this library :-), but for 5-10% (I checked, that's the real impact - I'm not guessing) and never having to worry, or document a thing, I'd like to just merge as is

@m4tt72
Copy link
Collaborator

m4tt72 commented Jul 10, 2019

Humm, since you both agree on that, and this will definitely make the code more robust, then sure I'd rather have it working in all cases even if we sacrifice some of the performance...
I was already planning on implementing something that might make it faster but I'm not sure 🤞

@mikehardy
Copy link
Owner

Thanks @m4tt72 - during the initial bits of transition as we hit all the edge cases I want more than anything to have this be at least one part of the rn60 experience that isn't frustrating. I'm actually going through it myself on my work app and it's basically a mess. But at least jetifier is working :-) and it will still be plenty fast with this I think, based on your previous work

@mikehardy mikehardy merged commit 7b5f4a4 into mikehardy:master Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants