Skip to content
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

Prevent Yarn from rewriting the lockfile when switching between online ⇄ offline #51

Merged
merged 3 commits into from
Mar 23, 2017

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Mar 7, 2017

First draft cc @bestander

@arcanis arcanis changed the title Create 0000-offline-resolution-field.md Prevent Yarn from rewriting the lockfile when switching between online ⇄ offline Mar 7, 2017
@cpojer
Copy link

cpojer commented Mar 7, 2017

This is great!


Yarn currently has two different type of values for the lockfile `resolved` fields:

- When online, they're in the form `${source}/${name}-${version}.tar.gz#${hash}`
Copy link
Member

Choose a reason for hiding this comment

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

also it can be a git URL e.g. ssh://[email protected]/bestander/test-repo.git, a link: or a file:.


- **Rational: Why removing the source part of the resolved field?**

The source is redundant, and misleading:
Copy link
Member

Choose a reason for hiding this comment

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

Another advantage is that Yarn can start downloading the dependencies immediately skipping the resolution step.
Also there may be some caveats regarding proxies and private packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also there may be some caveats regarding proxies and private packages.

It might also causes issue on this regard even with the source; for example, a dev might use an internal IP as mirror, and a public URL on CI. Baking the source in the lockfile means that this cannot be done without postprocessing the lockfile (since the CI won't have access to this URL). Removing the source would help resolve this issue, since then the CI would use whatever registry it is configured to use rather than the ones specified in the lockfile.

Copy link
Member

Choose a reason for hiding this comment

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

Agree but it is a separate issue/feature

@bestander
Copy link
Member

I wonder if getting rid of domain from URL and making yarn.lock not changed when using Offline Mirror are two different problems you are trying to tackle.
Would it be simpler to do deal with them separately?

@arcanis
Copy link
Member Author

arcanis commented Mar 7, 2017

A first iteration could uniformize the two processes by baking the source into both offline & online package resolutions. There might be a bit less code involved, but I'm not completely sure (because there wouldn't be any distinction between offline and online, we would have to check the offline cache anyway to see if a matching file exists). The only step that would be avoided would be to dynamically resolve the source against the package.json (because then we would just use whatever source is set in the lockfile instead).

@bestander
Copy link
Member

Can you give an example how yarn.lock will look?

@arcanis
Copy link
Member Author

arcanis commented Mar 7, 2017

If we strip the source: (note that even the git repo is only resolved to its name + hash)

"@manaflair/term-strings@https://github.com/manaflair/term-strings.git#0782bd21a953f29f6425412865b826a62a06722c":
  version "0.7.0"
  resolved "term-strings-0782bd21a953f29f6425412865b826a62a06722c.tar.gz#0782bd21a953f29f6425412865b826a62a06722c"
  dependencies:
    babel-cli "^6.18.0"
    color-diff "^1.0.0"
    core-js "^2.4.1"

abbrev@1:
  version "1.1.0"
  resolved "abbrev-1.1.0-d0554c2256636e2f56e7c2e5ad183f859428d81f.tgz#d0554c2256636e2f56e7c2e5ad183f859428d81f"

ajv@^4.9.1:
  version "4.11.4"
  resolved "ajv-4.11.4-ebf3a55d4b132ea60ff5847ae85d2ef069960b45.tgz#ebf3a55d4b132ea60ff5847ae85d2ef069960b45"
  dependencies:
    co "^4.6.0"
    json-stable-stringify "^1.0.1"

If we forcefully write the source even when offline:

"@manaflair/term-strings@https://github.com/manaflair/term-strings.git#0782bd21a953f29f6425412865b826a62a06722c":
  version "0.7.0"
  resolved "https://github.com/manaflair/term-strings.git#0782bd21a953f29f6425412865b826a62a06722c"
  dependencies:
    babel-cli "^6.18.0"
    color-diff "^1.0.0"
    core-js "^2.4.1"

abbrev@1:
  version "1.1.0"
  resolved "https://registry.yarnpkg.com/abbrev/-/abbrev-1.1.0.tgz#d0554c2256636e2f56e7c2e5ad183f859428d81f"

ajv@^4.9.1:
  version "4.11.4"
  resolved "https://registry.yarnpkg.com/ajv/-/ajv-4.11.4.tgz#ebf3a55d4b132ea60ff5847ae85d2ef069960b45"
  dependencies:
    co "^4.6.0"
    json-stable-stringify "^1.0.1"

Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

I think these are all good ideas that should be evaluated separately.
Let's also focus on speed and backwards compatibility.


- **Rational: Why removing the source part of the resolved field?**

The source is redundant, and misleading:
Copy link
Member

Choose a reason for hiding this comment

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

Agree but it is a separate issue/feature


- If we change our registry URL, the package itself will not have changed. The lockfile should not be modified.

- The source is already in the package.json file - we can use it when we need to, since we will have checked by then that the resolved version still match (when working with a registry). Worst case scenario (direct link) we will catch that the file will have changed when checking the hash - but that's not something we could have avoided with storing the source in the lockfile anyway.
Copy link
Member

Choose a reason for hiding this comment

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

The source at package.json is loose that is resolved to a fixed URL in package-resolver.
Yarn gains speed by caching and freezing resolution step in the lockfile


- The source is already in the package.json file - we can use it when we need to, since we will have checked by then that the resolved version still match (when working with a registry). Worst case scenario (direct link) we will catch that the file will have changed when checking the hash - but that's not something we could have avoided with storing the source in the lockfile anyway.

- (Optional) Splitting the resulting `resolved` field into two others: `hash` (the hash of the tarballs) and `tarball` (the tarball name).
Copy link
Member

Choose a reason for hiding this comment

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

That might be useful especially that we want to migrate to sha2/sha3


- (Optional) Splitting the resulting `resolved` field into two others: `hash` (the hash of the tarballs) and `tarball` (the tarball name).

- Adding the hash to the tarball name. This is to prevent issues where two projects are being worked on at the same time, both with the same dependency `foobar`, except that one of those projects is using the development branch of `foobar` (possibly fetching the project directly from git). In such a case, both projects lockfile would resolve to the same dependency name, the same dependency version (because maintainers don't usually bump version number until the release), but different hashes.
Copy link
Member

Choose a reason for hiding this comment

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

Nice idea

@arcanis
Copy link
Member Author

arcanis commented Mar 22, 2017

I opened a PR for review (yarnpkg/yarn#2970) and updated the rfc (fortunately, there was more to remove than to add 😄).

@bestander
Copy link
Member

This looks good.
Can you address git dependencies in the RFC?
There we did not just strip source we actually generated a whole new file name some times.

@bestander bestander merged commit 44f84bb into yarnpkg:master Mar 23, 2017
@bestander
Copy link
Member

Great job, one big source of inconsistent caches is about to go away!

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.

3 participants