Fix devDependencies being installed when passing --production

[olingern]

Quick fix for #3630

Sypnosis:
Passing --production flag to yarn installs currently still installs devDependencies

Notes:
This is a quick fix. I think there could be a good refactor to move dependency retrieval out of fetchRequestFromCwd for testability, and reduce the number of states that pushDeps tries to handle at the moment.

Let me know if merging this now is good, or if I should break this apart / add tests.

Cause:
devDependencies is always called, and the var, isUsed, should handle this -- but the early return never happens due to ignoreUnusedPatterns evaluating to false.

pushDeps('dependencies', projectManifestJson, {hint: null, optional: false}, true);
pushDeps('devDependencies', projectManifestJson, {hint: 'dev', optional: false}, !this.config.production);
pushDeps('optionalDependencies', projectManifestJson, {hint: 'optional', optional: true}, true);

const pushDeps = (depType, manifest: Object, {hint, optional}, isUsed) => {
  if (ignoreUnusedPatterns && !isUsed) {
    return;
  }
  ...

[redreboot]

https://travis-ci.org/prettier/eslint-config-prettier.svg?branch=master

[BYK]

Hi @olingern, thanks a lot for the PR! That said this is not the correct approach at all. Let me elaborate.

Passing --production flag to yarn installs currently still installs devDependencies

It does not. It just resolves these dependencies too to get a complete and consistent picture of all dependencies. See #3630 (comment)

Unfortunately your diagnosis is not correct. I have just tested this locally and devDependencies are not installed when I pass the --production flag. As mentioned earlier, the core behavior is not buggy. The only thing that may need fixin is yarn trying to resolve devDependencies when in production mode. This resolution is required to ensure a consistent install across all builds but as @arcanis mentioned, we may add some hoops and possibly extra information to the lockfile to prevent even the resolution from happening.

In its current form, this PR is not mergable because it actually breaks a core functionality as you can tell from the failing unit tests. What needs to be done is preventing resolution requests from happening for devDependencies. That information though, should still be available to us, possibly from the lockfile.

This may not be an easy thing to solve TBH :(

[olingern]

Unfortunately your diagnosis is not correct. I have just tested this locally and devDependencies are not installed when I pass the --production flag. As mentioned earlier, the core behavior is not buggy. The only thing that may need fixin is yarn trying to resolve devDependencies when in production mode. This resolution is required to ensure a consistent install across all builds but as @arcanis mentioned, we may add some hoops and possibly extra information to the lockfile to prevent even the resolution from happening.

I see. Looking at this more closely, looks to be an oversight on my part :(

What needs to be done is preventing resolution requests from happening for devDependencies. That information though, should still be available to us, possibly from the lockfile.

I see. So, this block below from src/cli/commands/install.js should have devDependencies already filtered out somehow (maybe with additional info added to the lockfile) when the production flag is passed to avoid package resolutions? I still could be making this too simplistic.

  steps.push(async (curr: number, total: number) => {
    this.reporter.step(curr, total, this.reporter.lang('resolvingPackages'), emoji.get('mag'));
    await this.resolver.init(this.prepareRequests(depRequests), {
      isFlat: this.flags.flat,
      isFrozen: this.flags.frozenLockfile,
      workspaceLayout,
    });
    topLevelPatterns = this.preparePatterns(rawPatterns);
    flattenedTopLevelPatterns = await this.flatten(topLevelPatterns);
    return {bailout: await this.bailout(topLevelPatterns, workspaceLayout)};
  });

If this is a behemoth of a task to take on, I can close this out for now

[BYK]

If this is a behemoth of a task to take on, I can close this out for now

It may be. Sorry, we were too busy working towards 1.0 tasks, hence the radio silence. Will come back to this in a few weeks.

[BYK]

@olingern closed this a day ago

😢

[olingern]

@BYK Not to worry :) I'd love to work on this, but I think my initial diagnosis of the problem was off the mark. It's probably easier / makes more sense to start anew when there's more of a consensus on a solution

we may add some hoops and possibly extra information to the lockfile to prevent even the resolution from happening.

Ping me on Discord @olingern if it's easier to chat there :)