chore: remove fs-extra #1271

JCMais · 2020-09-23T02:24:22Z

- Summary

See issue above.

I've left a few comments about some changes that need to be addressed before this is merged.

I also wrote this using Node.js 14, so I need to run the tests again using Node.js 8 as it's probably going to yell at me. 😄 I will do that when I find some time tomorrow.

- Test plan

This PR adds no new features/changes other than removing all usages of fs-extra, so the existing test suite was sufficient.

There are some tests failing locally here, but it should not be related to these changes, as they were already failing before I started working on this. I will take a look later into this.

- Description for the changelog

remove fs-extra and use util.promisify instead

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

site/sync.js

site/watch.js

src/commands/functions/create.js

src/lib/exec-fetcher.js

src/lib/exec-fetcher.test.js

src/utils/proxy.js

tests/utils/siteBuilder.js

JCMais · 2020-09-23T02:46:25Z

tests/utils/siteBuilder.js

@@ -22,74 +33,75 @@ const createSiteBuilder = ({ siteName }) => {
      const dest = path.join(directory, pathPrefix, 'netlify.toml')
      const content = toToml(config, { space: 2 })
      tasks.push(async () => {
-        await fs.ensureFile(dest)
+        await ensureDir(path.dirname(dest))


We only need to ensure that the directory is created, as writeFile already creates the file if it does not exists (default mode is 'w').

tests/utils/siteBuilder.js

src/lib/exec-fetcher.test.js

erezrokah

Thanks @JCMais, nice work.
Added some comments.

site/sync.js

src/commands/functions/create.js

src/lib/exec-fetcher.js

src/lib/exec-fetcher.test.js

src/lib/http-agent.js

src/utils/proxy.js

tests/utils/siteBuilder.js

JCMais · 2020-09-23T18:14:45Z

@erezrokah @ehmicky I've addressed the requested changes. 👍

del could have been a dev dependency, as it's only used in tests, but I decided to create a separated function inside lib/fs.js just in case it would be useful later on. This is the reason it is a normal dependency.

Almost all tests are green here now, the tests that were failing before were due to missing the NETLIFY_AUTH_TOKEN environment variable. The only one not working now is the one related to Edge Handlers, most probably because I don't have access to the preview.

I also took the opportunity to do a small refactor on site/{sync,watch}.js to use async/await instead of .then() and also created a separated fs file with some utils, this is the second commit above (they are only used inside watch.js and sync.js).

package.json

tests/utils/siteBuilder.js

erezrokah

Thanks for the quick follow up @JCMais, add a few minor comments and suggestions.
You're correct about Edge Handlers, they are not available for all accounts yet.

site/fs.js

erezrokah · 2020-09-24T13:11:54Z

site/fs.js

+    // ignore erros for mkdir
+  }
+
+  const childrenItems = await fs.readdir(src)


This has the potential to create a lot of promises.
I don't think we should optimise this yet since it's only used for our docs site that doesn't contain many of files.

src/commands/functions/create.js

erezrokah · 2020-09-24T13:18:07Z

src/lib/exec-fetcher.js

-  const binExists = await fs.exists(binPath)
-  if (!binExists) {
-    return false
+  try {


Nice cleanup!

src/lib/fs.js

src/utils/proxy.js

tests/utils/siteBuilder.js

JCMais · 2020-09-24T15:18:16Z

Rebased on top of master and addressed all your comments @erezrokah, thanks for the review!

site/fs.js

JCMais · 2020-09-24T17:27:18Z

By the way, if we look further into this change, it probably makes sense to use graceful-fs directly instead of just fs. As just using fs is not going to handle some scenarios that were being handled before when using fs-extra (which uses graceful-fs internally).

v4 does not monkey-patch fs anymore.

edit: and from looking at fs-extra it seems that it was already using graceful-fs v4: https://github.com/jprichardson/node-fs-extra/blob/b7df7cce3f7ca5bc0ab85110aa997bd0ad33482f/package.json#L39

with the above in mind, I don't know if this PR makes sense anymore, given the original context. If it does, let me know and I will finish the remaining requested changes.

erezrokah · 2020-09-28T12:34:48Z

@JCMais, this PR brings value in the form of:

Doing some refactoring and wrapping fs with our own layer (making it easier to make the relevant changes once we drop Node 8 support).
Using Node core when possible and falling back to specific dependencies when needed seems to make more sense in our case.

It would be great if you could follow up with the remaining changes.

JCMais · 2020-09-28T12:57:13Z

Thanks @erezrokah, I will do that as soon as I get a chance today.

JCMais · 2020-09-29T14:11:10Z

added cpy as a dependency to ./site and used it instead of the custom implementation.

The test failure is weird, I will try running the project on Windows to take a look if it's also reproducible locally (looks like there are a few EPERM / EBUSY errors too).

I have not rebased with master yet, will do after the latest changes are reviewed.

erezrokah · 2020-09-30T12:50:22Z

I have not rebased with master yet, will do after the latest changes are reviewed.

Latest changes are good, thanks! Can you follow up with a rebase? I'm re-running the tests to verify

site/fs.js

JCMais · 2020-09-30T13:06:31Z

the branch has been rebased with master

JCMais · 2020-10-06T14:02:03Z

Resolved the conflict directly in the GitHub UI (thus the merge commit), if you need me to rebase with master just let me know.

erezrokah · 2020-10-06T14:10:50Z

Sorry for the delay with this @JCMais, going to merge and publish after https://jamstackconf.com/virtual/

JCMais · 2020-10-06T14:18:21Z

no hurries @erezrokah , looking forward to the conf too. 😄

create a separated file for fs utils

Co-authored-by: ehmicky <[email protected]>

erezrokah

Thank you for your patience with this @JCMais

JCMais commented Sep 23, 2020

View reviewed changes

site/sync.js Outdated Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

site/watch.js Outdated Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

src/commands/functions/create.js Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

src/lib/exec-fetcher.js Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

src/lib/exec-fetcher.test.js Outdated Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

src/utils/proxy.js Outdated Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

tests/utils/siteBuilder.js Outdated Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

tests/utils/siteBuilder.js Outdated Show resolved Hide resolved

ehmicky reviewed Sep 23, 2020

View reviewed changes

tests/utils/siteBuilder.js Outdated Show resolved Hide resolved

ehmicky reviewed Sep 23, 2020

View reviewed changes

src/lib/exec-fetcher.test.js Outdated Show resolved Hide resolved

erezrokah suggested changes Sep 23, 2020

View reviewed changes

JCMais force-pushed the chore-remove-fs-extra branch from b72ba58 to 453ccb9 Compare September 23, 2020 18:07

JCMais mentioned this pull request Sep 23, 2020

refactor: use async fs calls in src/utils/proxy.js #1275

Closed

ehmicky reviewed Sep 23, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

JCMais commented Sep 23, 2020

View reviewed changes

tests/utils/siteBuilder.js Outdated Show resolved Hide resolved

JCMais marked this pull request as ready for review September 23, 2020 18:49

erezrokah added the type: chore work needed to keep the product and development running smoothly label Sep 24, 2020

erezrokah suggested changes Sep 24, 2020

View reviewed changes

JCMais force-pushed the chore-remove-fs-extra branch from 22bca29 to 814ba72 Compare September 24, 2020 15:14

ehmicky reviewed Sep 24, 2020

View reviewed changes

site/fs.js Show resolved Hide resolved

ehmicky reviewed Sep 24, 2020

View reviewed changes

site/fs.js Show resolved Hide resolved

ehmicky reviewed Sep 24, 2020

View reviewed changes

site/fs.js Show resolved Hide resolved

ehmicky reviewed Sep 30, 2020

View reviewed changes

site/fs.js Show resolved Hide resolved

ehmicky approved these changes Sep 30, 2020

View reviewed changes

JCMais force-pushed the chore-remove-fs-extra branch from d486341 to 54403f4 Compare September 30, 2020 13:06

JCMais and others added 5 commits October 11, 2020 17:27

chore: remove fs-extra and use promisifed fs.* fns instead

4c72e36

chore: remove fs-extra from site/{watch,sync}.js

2816458

create a separated file for fs utils

chore: make it explicit that mkdirRecursiveAsync is async

be617a6

chore: use lib/fs.js in lib/http-agent.js

4569799

chore: fix semver range for del dependency

6c2a274

Co-authored-by: ehmicky <[email protected]>

erezrokah force-pushed the chore-remove-fs-extra branch from 106d407 to 6c2a274 Compare October 11, 2020 15:29

JCMais requested a review from a team as a code owner October 11, 2020 15:29

chore: fix lint error

86b3ba4

erezrokah approved these changes Oct 11, 2020

View reviewed changes

erezrokah merged commit 91b147e into netlify:master Oct 11, 2020

JCMais deleted the chore-remove-fs-extra branch October 13, 2020 12:56

chore: remove fs-extra #1271

chore: remove fs-extra #1271

Uh oh!

Conversation

JCMais commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JCMais Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JCMais commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erezrokah Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erezrokah Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JCMais commented Sep 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JCMais commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah commented Sep 28, 2020

Uh oh!

JCMais commented Sep 28, 2020

Uh oh!

JCMais commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah commented Sep 30, 2020

Uh oh!

Uh oh!

JCMais commented Sep 30, 2020

Uh oh!

JCMais commented Oct 6, 2020

Uh oh!

erezrokah commented Oct 6, 2020

Uh oh!

JCMais commented Oct 6, 2020

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

JCMais commented Sep 23, 2020 •

edited

Loading

JCMais Sep 23, 2020 •

edited

Loading

JCMais commented Sep 23, 2020 •

edited

Loading

JCMais commented Sep 24, 2020 •

edited

Loading

JCMais commented Sep 29, 2020 •

edited

Loading