Remove failed to install optional modules from node_modules #2375
Conversation
lgeiger
force-pushed
the
cleanup
branch
2 times, most recently
from
abf18e2 to
44b4e47
Compare
lgeiger
changed the title
There was a problem hiding this comment.
This PR is now ready to review 🎉
This behavior is important if one tries to rebuild native dependencies for the usage in Electron with electron-builder since it'll abort if the failing optional module is still present. For more informations see: electron-userland/electron-builder#1075
| assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'sub-dep'))); | ||
| return runInstallWithoutCheck({}, 'should-install-failing-optional-sub-deps', (config) => { | ||
| assert.equal(fsNode.existsSync(path.join(config.cwd, 'node_modules', 'optional-failing')), false); | ||
| assert.equal(fsNode.existsSync(path.join(config.cwd, 'node_modules', 'sub-dep')), true); |
There was a problem hiding this comment.
Sub-dependencies only required by the failing module still aren't deleted.
| return runInstall({}, 'should-install-failing-optional-deps', async (config) => { | ||
| assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'optional-failing'))); | ||
| return runInstallWithoutCheck({}, 'should-not-install-failing-optional-deps', (config) => { | ||
| assert.equal(fsNode.existsSync(path.join(config.cwd, 'node_modules', 'optional-failing')), false); |
There was a problem hiding this comment.
Why did you switch to existsSync from await fs.exists?
There was a problem hiding this comment.
I did it primarily out of two reasons:
- If I recall correctly
fs.existsthrows if the path is empty fs.existsis deprecated whilefs.existsSyncisn't
There was a problem hiding this comment.
In Yarn we have our own wrapper to fs methods and fs.exists returns a Promise that is async/awaitable, it is not deprecated afaik.
For the async io operations we prefer using async/await everywhere, it is better to follow a single guideline for the project
There was a problem hiding this comment.
The wrapped method uses Node's fs.exists underneath which is deprecated: https://nodejs.org/api/fs.html#fs_fs_exists_path_callback
That said I'm happy to change it for consistency 👍
There was a problem hiding this comment.
Thanks, I did not know that.
We'll probably do a one-off codemod to change to fs.access everywhere
| @@ -28,6 +28,18 @@ export const runInstall = run.bind( | |||
| [], | |||
| ); | |||
|
|
|||
| export const runInstallWithoutCheck = run.bind( | |||
There was a problem hiding this comment.
Why would you need install without check?
There was a problem hiding this comment.
Since the optional-failing module will be removed from node_modules, check will fail.
There was a problem hiding this comment.
That might be a problem, check should not fail after a correct install
There was a problem hiding this comment.
Oh I see. So just removing the folder from node_modules isn't enough and will cause check to fail since the module isn't installed.
|
I suppose there needs to be a fix in Check that would ignore optional deps
if they are missing
…On Wed, 4 Jan 2017 at 12:07, Lukas Geiger ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In __tests__/commands/_helpers.js
<#2375>:
> @@ -28,6 +28,18 @@ export const runInstall = run.bind(
[],
);
+export const runInstallWithoutCheck = run.bind(
Oh I see. So just removing the folder from node_modules isn't enough and
will cause check to fail since the module isn't installed.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2375>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBdWBFryuusl7g4Xw_SQ6GC_AaIPWqdks5rO4ubgaJpZM4LaFtC>
.
|
|
What's reflected in the |
|
Lock file represents full dependency graph. |
|
Ok, that makes sense since many times the optional dependencies are platform specific modules (darwin, windows, linux, etc.). Thanks @bestander and @lgeiger! |
Summary
Yarn now removes failed to install optional modules from
node_modules.Addresses #2274
Test plan
I manually tested this PR and it works as expected.
Unfortunately the integration tests fail but I can't figure out why. Any help would be really appreciated.The integration tests have been adjusted accordingly.