feat(type): add support for new link: dependency type #3359

mgcrea · 2017-05-09T15:33:22Z

Summary

Following the accepted 0000-link-dependency-type.md RFC, and as discussed in yarnpkg/rfcs#34, here is a rebase of my previous work implementing both:

Support for a new link: specifier
CLI flag --linkFileDependencies to override file: specifier handling.

Test plan

Did not port previous unit test back for now as the idea is to first discuss the implementation with @arcanis and @bestander. Code is very likely to change. Will happily add unit tests once the implementation looks OK.

After rebasing the PR, tests are currently failing (master is failing too so probably ok).

bestander · 2017-05-09T16:22:27Z

Awesome!
Thanks

arcanis · 2017-05-09T18:00:49Z

Great PR! Seems good to merge, except for the way you're accessing the command line parameter -
you should use the config object to store it, and retrieve it from there later (unless there's a reason why you can't?). Otherwise, it won't work properly with the .yarnrc command line arguments :)

bestander · 2017-05-11T13:41:21Z

ping @mgcrea, we'd love to have it in 0.25 that we plan to cut tomorrow

benlangfeld · 2017-05-11T14:07:40Z

@arcanis / @bestander Is there precedent for that storage in config that we could follow?

arcanis · 2017-05-11T14:14:06Z

Sure, you can take a look at this commit, for example. You just have to use getOption in the Config's init() method to set the variable to the right value, and after that you can access it from the config object (usually available via this.config).

benlangfeld · 2017-05-11T15:05:42Z

@arcanis Is this what you had in mind?

arcanis · 2017-05-11T18:26:25Z

@benlangfeld Not exactly, but not far! Look at the commit I just pushed if you're interested; basically, instead of adding a linkFileDependencies argument to the resolver, we just use the config object as source of truth.

@mgcrea I've added a test to make sure that everything works, and unfortunately it seems there's still a few issues (I quickly checked, and I think one of them is because the install command is trying to read the manifest from the location. The other one is inside the check command, which fails because it is trying to make sure that the package path exists, which might not be true if the link is a broken link, or if the link target has no package.json). Can you take a look at this?

Don't bother about the rebase, I'll fix them directly on Github before merging.

bbrzoska · 2017-05-22T15:04:48Z

I've tested this branch and the package name is not inferred from the path:

$ yarn add link:../i18n-import-webpack-plugin --dev
yarn add v0.25.0-0
[1/4] 🔍  Resolving packages...
error Package "@0.0.0" doesn't have a "name".

I have to run the following instead:

yarn add i18n-import-webpack-plugin@link:../i18n-import-webpack-plugin --dev

The first command works with file: specifier.

arcanis · 2017-05-22T15:07:08Z

Ping @mgcrea ? 🙂

mgcrea · 2017-05-23T09:26:52Z

@arcanis I'll have a look today. Thanks for the patches.

mgcrea · 2017-05-23T11:45:23Z

Did finish a first squash+rebase+update, had to add code since the functionality was broken by a few recent commits on master (mostly from the cli/commands/check.js related checks), I've kept the commit separated for now to ease the review of these new patches since initial commit. Some tests seems a bit hacky, but tried to keep the refactoring to a minimum.

Looks like my existing workflow is still broken on master, so I'll have to dig a bit deeper to make sure I have the relevant unit tests.

bestander · 2017-05-25T16:40:40Z

Thanks, @mgcrea

bestander · 2017-05-25T16:43:24Z

src/package-fetcher.js

@@ -37,6 +37,13 @@ export default class PackageFetcher {
    const dest = this.config.generateHardModulePath(ref);

    const remote = ref.remote;
+
+    // Mock metedata for linked dependencies
+    if (remote.type === 'link') {


Would it be a bit cleaner if LinkFetcher existed instead?

@bestander iirc at the time I started to work on the feature, I had to diverge earlier in the stack so there were no use for a LinkFetcher. Might be worth revisiting though.

mgcrea · 2017-05-26T15:45:54Z

I think we're getting close to something "ready". I've fixed and added a unit-test for the issue encountered by @bazyli-brzoska. Test are OK, rebase done.

@bestander, regarding the package-fetcher.js patch, I tried again to use a LinkFetcher but it would have still required a lot of branching in PackageFetcher as we need to opt out cache entirely. So I decided against it (would lead to more LOCs & increased complexity).

Let me know if there is something else I can do.

bestander · 2017-05-30T09:47:57Z

@mgcrea, I agree about the fetcher, makes sense to have an if block rather than more complexity.

bestander · 2017-05-30T10:35:30Z

Amazing work, @mgcrea!
You've lead the whole feature from design to implementation.
I am sure quite a lot of people would want to use it if they know about it, do you want to post in the blog about it?

bestander · 2017-05-30T10:38:07Z

It is ready to merge although windows tests fail:

__tests__\commands\install\integration.js (120.573s)
  ● creates a symlink to a non-existing directory when using the link: protocol
    Error: expect(received).toEqual(expected)
    
    Expected value to equal:
      "../baz"
    Received:
      "C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\yarn-install-link-1495898245160-0.05036491088610817\\baz\\"

mgcrea · 2017-05-30T10:45:20Z

@bestander can fix the tests (add a platform ternary) if you want.

Blogpost is a great idea, I'll try to come up with something.

bestander · 2017-05-30T10:46:04Z

If you could fix it, please go ahead

…

On 30 May 2017 at 11:45, Olivier Louvignes ***@***.***> wrote: @bestander <https://github.com/bestander> can fix the tests (add a platform ternary) if you want. Blogpost is a great idea, I'll try to come up with something. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3359 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWGEj7B9kabAT43YNxPbMZKLMxUcLks5r-_NKgaJpZM4NVfc6> .

mgcrea · 2017-05-30T10:46:55Z

src/cli/commands/check.js

@@ -49,21 +49,33 @@ export async function verifyTreeCheck(
  const dependenciesToCheckVersion: PackageToVerify[] = [];
  if (rootManifest.dependencies) {
    for (const name in rootManifest.dependencies) {
+      const version = rootManifest.dependencies[name];
+      // skip linked dependencies
+      const isLinkedDepencency = /^link:/i.test(version) || (/^file:/i.test(version) && config.linkFileDependencies);


@bestander This is the last thing I don't really like, as regex testing feels a bit hackish, but I would need to change the parent method API to pass the proper type. So was unsure if it was worth it.

mgcrea · 2017-05-30T13:22:38Z

I've fixed the tests for windows, (looks like the test are still failing there but for unrelated reasons).

bestander · 2017-05-30T13:39:11Z

Thanks, yeah one test got broken earlier, investigating

zertosh · 2017-05-30T14:33:46Z

https://github.com/zertosh/yarn-link-test is an example of how link breaks down due to Node's realpath module resolution (nodejs/node#3402)

benlangfeld · 2017-05-30T14:59:02Z

Much ❤️ to everyone involved here, particularly @mgcrea. You guys are awesome and this will make my team very happy. We owe you at least some drinks!

benlangfeld · 2017-06-02T16:57:57Z

@bestander I guess this will now go in 0.26.0? By the recent cadence of minor releases, that would likely be cut within the next two weeks. Is that a fair estimate?

bestander · 2017-06-02T17:01:41Z

We plan to cut on Monday.
There are a couple of hi pri issues we want to fix before cutting 0.26.

felixrabe · 2018-05-06T16:21:08Z

So ... is this documented somewhere and/or example usage available?

Is

"my-package": "link:../some/path",  // package.depencencies entry

equivalent to

cd ../some/path
yarn link
cd -
yarn link my-package

?

felixrabe · 2018-05-06T16:22:09Z

(Equivalent meaning: will both approaches result in the same symlink being present in ./node_modules?)

jRichardeau · 2018-12-13T17:26:31Z

Why is this feature not documented ? Can we use it safely ?

mgcrea mentioned this pull request May 9, 2017

:link dependencies yarnpkg/rfcs#34

Merged

bestander mentioned this pull request May 9, 2017

Workspaces issue progress #3294

Closed

22 tasks

benlangfeld mentioned this pull request May 11, 2017

Permits configuring file: link behaviour in .yarnrc mgcrea/yarn#1

Closed

mgcrea force-pushed the feat-884 branch 4 times, most recently from 7f3946f to 9210f9d Compare May 23, 2017 10:34

bestander reviewed May 25, 2017

View reviewed changes

mgcrea force-pushed the feat-884 branch 5 times, most recently from 6a52f8b to ae00d87 Compare May 26, 2017 15:41

mgcrea force-pushed the feat-884 branch 3 times, most recently from 50d81bc to 7ad1bec Compare May 27, 2017 15:13

bestander approved these changes May 30, 2017

View reviewed changes

mgcrea commented May 30, 2017

View reviewed changes

mgcrea force-pushed the feat-884 branch 3 times, most recently from 7370f51 to 38fb1b3 Compare May 30, 2017 12:16

Add support for new link: dependency type

0b79bc7

mgcrea force-pushed the feat-884 branch from 38fb1b3 to 0b79bc7 Compare May 30, 2017 12:51

bestander merged commit db5edea into yarnpkg:master May 30, 2017

alexsasharegan mentioned this pull request Jun 28, 2017

Yarn link & webpack 2 & windows bootstrap-vue/bootstrap-vue#568

Closed

mtraynham mentioned this pull request Nov 20, 2017

yarn probably shouldn't cache packages resolved with a file path #2165

Closed

adrian-gierakowski mentioned this pull request Mar 6, 2018

RFC: file: specifier changes npm/npm#15900

Closed

amiller-gh mentioned this pull request Mar 8, 2018

lerna link should support --reject-cycles flag lerna/lerna#1312

Closed

jamescdavis mentioned this pull request Sep 5, 2018

Use devDependencies for in-repo dummy addons. typed-ember/ember-cli-typescript#252

Merged

p12tic mentioned this pull request Jun 18, 2019

Document link: dependency type (#793) yarnpkg/website#969

Merged

pomek mentioned this pull request Sep 29, 2021

Implemented option to install dependencies using npm ckeditor/ckeditor5-package-generator#38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(type): add support for new link: dependency type #3359

feat(type): add support for new link: dependency type #3359

mgcrea commented May 9, 2017 •

edited

Loading

bestander commented May 9, 2017

arcanis commented May 9, 2017

bestander commented May 11, 2017

benlangfeld commented May 11, 2017

arcanis commented May 11, 2017 •

edited

Loading

benlangfeld commented May 11, 2017

arcanis commented May 11, 2017 •

edited

Loading

bbrzoska commented May 22, 2017

arcanis commented May 22, 2017

mgcrea commented May 23, 2017

mgcrea commented May 23, 2017 •

edited

Loading

bestander commented May 25, 2017

bestander May 25, 2017

mgcrea May 25, 2017

mgcrea commented May 26, 2017

bestander commented May 30, 2017

bestander commented May 30, 2017

bestander commented May 30, 2017

mgcrea commented May 30, 2017

bestander commented May 30, 2017 via email

mgcrea May 30, 2017

mgcrea commented May 30, 2017

bestander commented May 30, 2017

zertosh commented May 30, 2017

benlangfeld commented May 30, 2017

benlangfeld commented Jun 2, 2017

bestander commented Jun 2, 2017

felixrabe commented May 6, 2018

felixrabe commented May 6, 2018 •

edited

Loading

jRichardeau commented Dec 13, 2018

feat(type): add support for new link: dependency type #3359

feat(type): add support for new link: dependency type #3359

Conversation

mgcrea commented May 9, 2017 • edited Loading

bestander commented May 9, 2017

arcanis commented May 9, 2017

bestander commented May 11, 2017

benlangfeld commented May 11, 2017

arcanis commented May 11, 2017 • edited Loading

benlangfeld commented May 11, 2017

arcanis commented May 11, 2017 • edited Loading

bbrzoska commented May 22, 2017

arcanis commented May 22, 2017

mgcrea commented May 23, 2017

mgcrea commented May 23, 2017 • edited Loading

bestander commented May 25, 2017

bestander May 25, 2017

Choose a reason for hiding this comment

mgcrea May 25, 2017

Choose a reason for hiding this comment

mgcrea commented May 26, 2017

bestander commented May 30, 2017

bestander commented May 30, 2017

bestander commented May 30, 2017

mgcrea commented May 30, 2017

bestander commented May 30, 2017 via email

mgcrea May 30, 2017

Choose a reason for hiding this comment

mgcrea commented May 30, 2017

bestander commented May 30, 2017

zertosh commented May 30, 2017

benlangfeld commented May 30, 2017

benlangfeld commented Jun 2, 2017

bestander commented Jun 2, 2017

felixrabe commented May 6, 2018

felixrabe commented May 6, 2018 • edited Loading

jRichardeau commented Dec 13, 2018

mgcrea commented May 9, 2017 •

edited

Loading

arcanis commented May 11, 2017 •

edited

Loading

arcanis commented May 11, 2017 •

edited

Loading

mgcrea commented May 23, 2017 •

edited

Loading

felixrabe commented May 6, 2018 •

edited

Loading