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

Allow for installing dependencies without manifest #3624

Merged
merged 3 commits into from Jun 22, 2017

Conversation

sheerun
Copy link
Contributor

@sheerun sheerun commented Jun 12, 2017

This is continuation of #2651. Changes:

  1. Fix extracting default name from url so .git extension won't get into way
  2. Don't write any package.json, store default manifest in .yarn-metadata.json instead.
    Bonus: Cache normalized package.json in in .yarn-metadata.json, so installs should get faster.
  3. Ensure one can install named package like foobar@https://github.com/foo/bar.git
  4. Add support for file: resolver as well
  5. Fix yarn check so it allows for no package.json in installed dependencies

Any comments?

const manifest = await this.config.readManifest(loc, this.registry);
const manifest: Manifest = await (async () => {
try {
return await await this.config.readManifest(loc, this.registry);
Copy link
Member

Choose a reason for hiding this comment

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

await await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

async readManifest(dir: string, priorityRegistry?: RegistryNames, isRoot?: boolean = false): Promise<Manifest> {
const manifest = await this.maybeReadManifest(dir, priorityRegistry, isRoot);
readManifest(dir: string, priorityRegistry?: RegistryNames, isRoot?: boolean = false): Promise<Manifest> {
return this.getCache(`manifest-${dir}`, async (): Promise<Manifest> => {
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to move getCache from maybeReadManifest here?

Copy link
Contributor Author

@sheerun sheerun Jun 18, 2017

Choose a reason for hiding this comment

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

yes, it's because maybeReadManifest was called few times in the same process, and the getCache there memoized "nil" return value. base-fetcher/fetch method writes the manifest between these calls, so nil was returned even after manifest was written.

moving cache to readManifest fixes the issue as getCache doesn't cache rejected promises (as opposed to resolved nil values)

Copy link
Contributor Author

@sheerun sheerun Jun 18, 2017

Choose a reason for hiding this comment

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

Actually this was always a bug, but didn't surface. With this PR I actually expect manifest to change (be created) across reads, so I needed to fix this.

@bestander
Copy link
Member

@sheerun, thanks for catching this up.
One test got broken though nstall should write and read integrity file based on lockfile entries

Now that we allow for installing packages without package.json,
shallow integrity check will succeed if it's removed from package.

I fix the test by removing installed director instead.
@sheerun
Copy link
Contributor Author

sheerun commented Jun 18, 2017

@bestander I fixed the test. It's behavior changed, as if we allow for installing packages without package.json, simply removing package.json from installed dependency is not enough to fail shallow integrity check, we need to remove whole directory, and that's what I did to fix this test.

@bestander bestander merged commit b223d51 into yarnpkg:master Jun 22, 2017
@bestander
Copy link
Member

Great work, @sheerun!
That is some impressive change across many layers of code.
Anything else you'd like to help out with? :)

GulajavaMinistudio added a commit to GulajavaMinistudio/yarn that referenced this pull request Jun 22, 2017
Allow for installing dependencies without manifest (yarnpkg#3624)
@andreypopp
Copy link

Oh wow! Thank you for that!

@sheerun
Copy link
Contributor Author

sheerun commented Jun 22, 2017

@bestander How about support for selecting a tag from git repository by semver selector?

Something like yarn add bestander/chrome-app-livereload@0.0.x or yarn add bestander/chrome-app-livereload#0.0.x depending on what you agree on

@sheerun
Copy link
Contributor Author

sheerun commented Jun 22, 2017

Probably # makes more sense, as then the source of package can be expressed as

NAME@[REGISTRY#]SEMVER, where registry is optional

in case of git repositories, the registry is repository itself, so

yarn add jquery@jquery/jquery#1.x

makes sense as it adds jquery in version 1.x from "jquery/jquery" registry (git tags), and

yarn add jquery@1.x

installs version 1.x from default "npm" registry

@bestander
Copy link
Member

The problem is that # is used for git hashes in git resolution logic.

@sheerun
Copy link
Contributor Author

sheerun commented Jun 23, 2017

This probably wouldn't be an issue as hashes look much different than semver. In Bower there's simple check for hash (4+ more word characters), and nobody had any issues: https://github.com/bower/bower/blob/master/lib/core/resolvers/GitResolver.js#L178

@bestander
Copy link
Member

@sheerun, that would be really great to have this feature.
Go ahead!

@sheerun
Copy link
Contributor Author

sheerun commented Jun 25, 2017

@bestander There was some change since my PRs on master branch, and now yarn add bestander/chrome-app-livereload fails with:

An unexpected error occurred: "https://github.com/bestander/chrome-app-livereload.git: Request failed "406 Not Acceptable"".

@bestander
Copy link
Member

@sheerun, there were not that many changes since the merge and latest CI build shows the tests passing https://circleci.com/gh/yarnpkg/yarn/4198

@bestander
Copy link
Member

bestander commented Jul 5, 2017

@sheerun, looks like the tests at __tests__/package-resolver.js are passing fine but the solution fails at end to end installation.
If you have a chance to have a look and fix the issue that would be really great, I think we need to cover this feature with a proper e2e test.

@sheerun
Copy link
Contributor Author

sheerun commented Jul 7, 2017

Yeah I was surprised these tests weren't e2e

@sheerun
Copy link
Contributor Author

sheerun commented Oct 2, 2017

@bestander @andreypopp Thanks to this patch bower-away can work: https://twitter.com/bower/status/914828104727703553 :)

@bestander
Copy link
Member

Thanks for sharing, @sheerun!

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.

None yet

3 participants