whyrusleeping / gx-go

gx subtool for golang
MIT License
80 stars 28 forks source link

link: override dependency versions with the current dependent package #51

Closed schomatis closed 6 years ago

schomatis commented 6 years ago

This PR extracts the --sync feature from https://github.com/whyrusleeping/gx-go/pull/48 (now named --override-deps) without the vendor/ support (that has some implementation problems, see https://github.com/whyrusleeping/gx-go/issues/46#issuecomment-416229688).

As with the imported PR, this command now requires gx-go link to be run inside the parent package of the dependency even though the link is still done to the global workspace. This breaks API but it makes it easier to reason about the code and about the link being done, and in most use cases the developer was already calling the command from a package anyway.

Closes #46.


link: add feature to override dependencies with linked packages

(This patch changes the current API.)

Add `--override-deps` flag to (in the case a of a dependency version mismatch)
replace the dependency version of the target package with the one in the current
dependent package. (Most of the implementation logic for this feature had to be
added to the `post-install` hook which also has a new `--override-deps` flag.)

Require the command to be run inside a package. Refactor the link/unlink
functions. Give more visibility to the options that allows specifying a
dependency to link by its name.
schomatis commented 6 years ago

@Stebalien PTAL. Although sub-optimal this still seems like a useful flag while developing across packages (especially for new developers who may not be familiarized with the gx-workspace and gx-update workflows).

schomatis commented 6 years ago

(Another possible enhancement for this PR or a new one: detect already linked packages and un-link+link them again. At the moment it would seem that the post-install hook + rewrite functionality doesn't re-process an already rewritten file -it's assuming that the import paths wont change?, but if we've updated something?- so if I link without --sync and it fails due to a mismatch version, when I link again, now with --sync, the rewrite part of the process won't happen but it won't fail either. The result is that both with and without the sync functionality will seem to fail -the user will need to actually un-link and re-link manually, but this won't be obvious to him/her-.)

Stebalien commented 6 years ago

detect already linked packages and un-link+link them again.

We should probably do that but we can do it in a new patch.

Stebalien commented 6 years ago

Thinking about this, we could actually use the dependencies specified in the original gxed package (the one we're replacing with a symlink). That would be the most conservative approach (effectively "downgrading" dependencies.

The only difference is that, if the target repo has added a new dependency also present in the current/parent package, we'll use the version in the target repo, not the current/parent. However, using the package.json of the current/parent isn't foolproof either (e.g., the target could have added a dependency that we don't have, one that depends on packages we do depend on but depends on different versions).

The other approach would be to do a fake gx-workspace update. However, that's way more work.

schomatis commented 6 years ago

The only difference is that, if the target repo has added a new dependency also present in the current/parent package, we'll use the version in the target repo, not the current/parent. However, using the package.json of the current/parent isn't foolproof either (e.g., the target could have added a dependency that we don't have, one that depends on packages we do depend on but depends on different versions).

Let's have both options: --deps=dependent and --deps=dependency (we can use other terms but let's keep them absolute, not relative to the current/mine/theirs perspective) and recommend the second one (and have it as the default) as the conservative approach (noting that it not foolproof).

Stebalien commented 6 years ago

Let's have both options: --deps=dependent and --deps=dependency (we can use other terms but let's keep them absolute, not relative to the current/mine/theirs perspective) and recommend the second one (and have it as the default) as the conservative approach (noting that it not foolproof).

In this case, the other strategy would really be "original" or "replaced" (dependency is could either mean the dependency that I'm replacing/linking or the one I'm linking to).

I'm really confused why you feel so strongly about keeping the terminology absolute. Personally, I find it extremely confusing (depending on the context). When I see an absolute term, I think "oh, they must not mean my current package, otherwise they would have just said that". Specifically, when I see "dependent", my first thought is "which dependent?".

schomatis commented 6 years ago

I'm really confused why you feel so strongly about keeping the terminology absolute. Personally, I find it extremely confusing (depending on the context).

Ok, let me rethink that and get back to you.

I think that my main objection is about the code and not the UX. From the point of the user who just runs cd package; gx-go link dependency I think you're right, "current" is the package we just moved to, there's no doubt there. Using "current" in the code starts losing meaning as we're constantly changing perspectives between dependency-dependent packages, and the original ("current") package from where we (the user) started has little to do with the ongoing operation of, for example, fetching the dependency of the the dependency. That's where I want to have a clear model of which package is on top from where we are extracting its dependencies to later convert (fetch) them into other (also) packages. But yes, that part of the code is a different realm from the command the user runs.

Stebalien commented 6 years ago

Ah, yes, that's totally reasonable. parent actually seems like a good word in that case (or root).

schomatis commented 6 years ago

@Stebalien Could you take another look please? Changed the terminology to target/current (using your command description) and renamed the option --override-deps. I've finally had to settle for just using the versions from the current dependent package (and didn't add the variation of taking them from the target package because this command overwrites it -since everything is done in the same global directory- and wanted to avoid changing too much functionality in this first iteration).

schomatis commented 6 years ago

@Stebalien

schomatis commented 6 years ago

@Stebalien

Stebalien commented 6 years ago

Ok, this looks correct. Sorry this took so long, I'm very unsure of how any of this code works so I kept on punting.