Skip to content

Conversation

@olingern
Copy link
Contributor

@olingern olingern commented May 14, 2019

Summary

Fixes #6040

Read my comment here for a summary of why and how I arrived at this.

Implementation details:

As mentioned in #6040, there are two entries for a given package being returned. So, this PR simply tracks dependencies by their pattern. This does mean that it's version specific, so this is an assumption on my part, i.e. if left-pad@1.0.0 is optional and left-pad@1.0.1 is a resolution -- left-pad@1.0.1 will still install

The fix:

  • Track deps by pattern
  • When building resolution deps, only add if package is not optional

I would consider this a WIP / draft since I will need to tackle tests and verify workspaces + resolutions are still intact.


Todos:

  • Get feedback
  • Determine if the existing local depMap var should be used somehow
  • Fix linting
  • Remove implementation comment?
  • Add tests for this case ( optional dependency / resolved )
  • Add to changelog

Test plan

Tests to add:

  • 1 optional dep, 1 resolution. Same versions
  • 1 optional dep, 1 resolution. Different versions

@buildsize
Copy link

buildsize bot commented May 14, 2019

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.11 MB 1.11 MB 205 bytes (0%)
yarn-[version].js 4.48 MB 4.48 MB 746 bytes (0%)
yarn-legacy-[version].js 4.67 MB 4.67 MB 875 bytes (0%)
yarn-v[version].tar.gz 1.12 MB 1.12 MB 2.3 KB (0%)
yarn_[version]all.deb 816.96 KB 817.07 KB 114 bytes (0%)

@olingern olingern changed the title Handle optional dependencies alongside resolutions [WIP] - Handle optional dependencies alongside resolutions May 14, 2019
Object.assign(this.resolutions, projectManifestJson.resolutions);
Object.assign(manifest, projectManifestJson);

this.resolutionMap.init(this.resolutions);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is moved to after pushDeps is so that we know what dependencies to not add

return;
}
const depMap = manifest[depType];
const workspaceName = manifest.name;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done to pass linter line length below

@olingern
Copy link
Contributor Author

Closing in favor of #7273

@olingern olingern closed this May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: "resolutions" trumps --ignore-optional

1 participant