Fix optional implicit dependencies #172
Conversation
nickpape
commented
May 3, 2017
nickpape
commented
- Add them to the list of implicitly pinned versions
- Do not cause shrinkwrap to fail
- Add to pinnedVersions as optional/normal dependency
|
VSO 346348 #Resolved |
| @@ -45,6 +45,16 @@ export enum InstallType { | |||
| UnsafePurge | |||
| } | |||
|
|
|||
| interface IImplicitVersionList { | |||
There was a problem hiding this comment.
IImplicitVersionList [](start = 10, length = 20)
We definitely need some JSDoc for these. Nobody besides you is going to know what they do :-) #Resolved
|
@pgonzal please take another look when you get time. #Resolved |
| * this interface records all version specifiers as well as whether | ||
| * all versions were optional or not. | ||
| */ | ||
| export interface IImplicitVersion { |
There was a problem hiding this comment.
export [](start = 0, length = 6)
Why are you exporting this?
If it's a private helper for building a data structure, then I guess I'm cool with it.
If you are really exporting it, then it should be a class with more well-defined semantics.
There was a problem hiding this comment.
good point. I think the below function needs to be private as well
In reply to: 114670964 [](ancestors = 114670964)
| @@ -56,25 +68,22 @@ export default class InstallManager { | |||
| /** | |||
| * Returns a map of all direct dependencies that only have a single semantic version specifier | |||
There was a problem hiding this comment.
direct dependencies that only have a single semantic version specifier [](start = 26, length = 70)
Is that the definition of "implicitly pinned version"?
What is the caller expected to do with the map?
What would happen if we didn't do anything with implicitly pinned dependencies?
This little hack is growing into a feature, so it would be helpful to have a little blurb somewhere that explains the problem and defines the terminology. When some threshold is reached, it probably needs to come out of InstallManager into its own class.
| }); | ||
|
|
||
| this._rushConfiguration.pinnedVersions.forEach((version: string, dependency: string) => { | ||
| pinnedVersions.set(dependency, version); | ||
| pinnedVersions.set(dependency, { |
There was a problem hiding this comment.
{ [](start = 37, length = 1)
You are basically simulating a class here
| pinnedVersions.forEach((version: string, dependency: string) => { | ||
| if (!shrinkwrapFile.hasCompatibleDependency(dependency, version)) { | ||
| pinnedVersions.forEach((info: IImplicitVersion, dependency: string) => { | ||
| const version: string = info.versions.values().next().value; |
There was a problem hiding this comment.
values().next() [](start = 46, length = 15)
I don't know what this code means
|
|
|
Pete and I talked about this... We think the idea of optional dependencies is fundamentally contradictory to using a shrinkwrap. If a Mac user adds an optional dependency (on say, FSEvents), then it will appear in shrinkwrap. However, this will cause it to fail on a Windows. Due to how shrinkwrap and optional deps work, if you add the dependency on a Windows PC, it will be missing from the shrinkwrap. If a shrinkwrap is present, NPM 4 will not install anything not specified in the shrinkwrap -- meaning that the Mac person will not get fsevents. You could say one option is that the info about whether the dependency is optional or not should be stored in the shrinkwrap. However, this is kind of odd because it means that the node_modules folder won't install styff that is listed in shrinkwrap -- and you have differing behaviours across platforms. Our solution was as follows... Since we already do NOT include fsevents in shrinkwrap (so as to not break Windows devs), we should:
In reply to: 299057564 [](ancestors = 299057564) |
|
Abandoning in favor of #175 |
