Skip to content

Conversation

@lukasholzer
Copy link
Contributor

🎉 Thanks for submitting a pull request! 🎉

Summary

Get rid of unnecessary dependency and replace with node native fs check


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures
    we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
    something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@lukasholzer lukasholzer requested review from danez and jobala January 27, 2023 11:06
@github-actions
Copy link
Contributor

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@danez
Copy link
Contributor

danez commented Jan 31, 2023

Do you think it is okay to use the synchronous version of this? I guess it won't be a big deal, but the CLI does have HTTP servers running, and sync stuff might block the event loop.

In the CLi this is used: https://github.com/netlify/cli/blob/main/src/lib/fs.mjs#L5-L12 which is pretty much what path-exists does
zisi uses path-exists

@lukasholzer
Copy link
Contributor Author

https://github.com/netlify/cli/blob/main/src/lib/fs.mjs#L5-L12

Node is single treaded so only because you put the async keyword in front of it it does not mean that it's run really asynchronously (parallel). IMHO this was a really bad language idea of JS but that said this won't block something.

Furthermore single async operations are slower than a synchronous one. (Especially for short directory exist checks this is the case.

Last but not least we have to be carefully what to use as cache utils are being executed with the users node version so any 3rd party library can be potentially harmful (they might drop support for node or whatever in a patch version and we don't know that)

Node fs.access was introduced in Node 10.0.0 whereas fs.existsSync is part since the beginning. (I give you the point that fs.access is more native but I bet you want feel any difference!

Except that synchronous code is often easier to read or to reuse

@danez
Copy link
Contributor

danez commented Jan 31, 2023

Node is single treaded so only because you put the async keyword in front of it it does not mean that it's run really asynchronously (parallel). IMHO this was a really bad language idea of JS but that said this won't block something.

Well async fs calls are indeed running in parallel to usercode, but I agree that blocking the event loop is probably more relevant to high throughput HTTP servers and similar.

Last but not least we have to be carefully what to use as cache utils are being executed with the users node version so any 3rd party library can be potentially harmful (they might drop support for node or whatever in a patch version and we don't know that)

I didn't think about this. 👍

@lukasholzer lukasholzer merged commit 026da36 into main Feb 1, 2023
@lukasholzer lukasholzer deleted the netlify-config-typescript-updates-remove-unneeded-dependency branch February 1, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants