-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[breaking?] - When using offline mirror yarn.lock does not change anymore #2970
Conversation
How does it handle backwards compatibility? |
Yep. Basically, if we have an URL, the offline cache filename is derived from the basename of this URL. If we have a path, then it is the offline cache filename. That should work with both old & new lockfiles. |
You have lint errors and 3 tests are failing. |
package.json
Outdated
@@ -15,6 +15,7 @@ | |||
"death": "^1.0.0", | |||
"debug": "^2.2.0", | |||
"detect-indent": "^5.0.0", | |||
"flow": "^0.2.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need flow in deps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried last friday to bump Flow's version to check if my issue was fixed in the new versions, but forgot to rollback the change.
src/fetchers/git-fetcher.js
Outdated
} else { | ||
return this.fetchFromExternal(); | ||
async getLocalAvailabilityStatus(): Promise<bool> { | ||
const tarballLegacyMirrorPath = this.getTarballMirrorPath({withCommit: false}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this justifies a comment explaining what is legacyMirrorPath and why we support it
src/fetchers/git-fetcher.js
Outdated
}); | ||
}); | ||
} | ||
|
||
async fetchFromExternal(): Promise<FetchedOverride> { | ||
const commit = this.hash; | ||
invariant(commit, 'Commit hash required'); | ||
const hash = this.hash; // if we don't store it in a temp variable, flow will not persist its invariant state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a common pattern in flow, you probably don't need to add a comment to describe it.
If someone decides to inline the variable flow won't let them anyway :)
Also I think we don't add comments on the same line with code (I might be wrong though)
👍 |
Does this mean that the description of the lockfile in the offline-mirror blog post isn't accurate anymore? If so, should probably either update the blog post, or port the content to the docs page that links to the blog post and update the content there |
Good find.
Would you want to help the project and send a PR with blog post changes?
…On Wed, 19 Apr 2017 at 15:55, Troy Astorino ***@***.***> wrote:
Does this mean that the description of the lockfile in the offline-mirror
blog post
<https://yarnpkg.com/blog/2016/11/24/offline-mirror/#initialize-the-new-lockfile>
isn't accurate anymore?
If so, should probably either update the blog post, or port the content to the
docs page that links to the blog post
<https://yarnpkg.com/lang/en/docs/offline-mirror/> and update the content
there
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2970 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACBdWPCgx0dBSPAOs224OGkn3RnZpJ8eks5rxpD9gaJpZM4MlP_D>
.
|
Sure, happy to give it a go! I'm somewhat new to yarn, and I stumbled on this trying to setup the offline mirror cache for a sprawling multi-package repo (haven't gotten it working yet), so I want to make sure I understand what's going on:
|
Doesn't seems like the blog post is up to date. I will create a PR for that |
Thanks!
cc me please
…On 25 April 2017 at 18:06, Dennis Yang ***@***.***> wrote:
Doesn't seems like the blog post is up to date. I will create a PR for that
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2970 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACBdWFWwNZH6eH9Dk3H3Kde6Z6E_If0yks5rzigBgaJpZM4MlP_D>
.
|
Does this solve: #3154? |
Git dependencies should still work but there can be an edge case where it broke, thanks for linking |
Another potentially related ticket: #3194 |
This change has broken git+https dependencies as described in #3309. There is a mismatch between the calculated hash and the expected hash. |
Yeah, we are on it |
For anyone else wondering if the docs for offline mirror are up-to-date, they were updated after this was merged |
Summary
Related rfc : yarnpkg/rfcs#51
When enabling the offline mirror, Yarn updates the lockfile by stripping the registry URL from its
resolved
fields. This feature aims to simplify this process by making such an update unneeded.Related issues: #393 / #394
Test plan
All tests should pass fine. I had a rewrite a few because of the nature of this change, but the fixtures stay the same (backward compatibility is preserved).