Skip to content

Conversation

@dav-is
Copy link
Contributor

@dav-is dav-is commented Feb 22, 2019

Replaces use of del in favor of rimraf package because it's smaller and less complex
Removes packages/next/lib/promisify.js in favor of native promisify because it's leftover Node 6 polyfill.
Converts 2 files to TypeScript

@wtgtybhertgeghgtwtg
Copy link
Contributor

Is there a reason for this change? It's not any simpler, since it has to be promisified, and del is still in the dependency tree via autodll-webpack-plugin and recursive-copy.

@timneutkens
Copy link
Member

Is there a reason for this change?

Actually it is simpler del exposes a async / sync api, uses globby and rimraf, the path to execution in this case is shorter and less code is loaded in eg next export

@timneutkens timneutkens reopened this Feb 22, 2019
@wtgtybhertgeghgtwtg
Copy link
Contributor

I agree del is much more bulky than it needs to be, but you are using the async API. And del will be loaded in next export anyway, since you're using recursive-copy.

@timneutkens
Copy link
Member

@wtgtybhertgeghgtwtg we're going to drop recursive-copy or use only parts of it too, it does way more than we need too 💯

@wtgtybhertgeghgtwtg
Copy link
Contributor

That seems reasonable. There is a chance that del will become less massive and potentially more performant than rimraf (when micromatch@4 drops and if fast-glob implements hasMagic). Would it be possible to document the reason behind these changes so that can be explored later?

@timneutkens
Copy link
Member

timneutkens commented Feb 22, 2019

We can even drop the globbing, we don't use the globbing part, we just want to delete 1 directory in this case 😅 Same goes for recursive copy, we literally only want to cp, or even mv the files to the out directory.

@wtgtybhertgeghgtwtg
Copy link
Contributor

Oh, wow, I should have noticed that.

@timneutkens
Copy link
Member

@wtgtybhertgeghgtwtg this is one of the reasons why I'm still not convinced moving to rimraf is even the correct thing to do, it could be even more performant to use fs.unlink directly in this case. But this is a good first step 👍

@timneutkens
Copy link
Member

timneutkens commented Feb 22, 2019

On a similar note we have the glob dep just to read pages/**/*, it could easily be switched to a fs.readdir version 🤔

@timneutkens
Copy link
Member

@wtgtybhertgeghgtwtg could you reach out to me on https://spectrum.chat/users/timneutkens / twitter.com/timneutkens?

@dav-is dav-is merged commit b146eb5 into canary Feb 22, 2019
@dav-is dav-is deleted the replace-del-rimraf branch February 22, 2019 19:49
@lock lock bot locked as resolved and limited conversation to collaborators Feb 22, 2020
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.

4 participants