pnpm / supi

Fast, disk space efficient installation engine. Used by pnpm
MIT License
24 stars 5 forks source link

feat: git dependencies can be declared with an alias name #16

Closed zkochan closed 6 years ago

zkochan commented 6 years ago

ref https://github.com/pnpm/pnpm/issues/838

cc @vjpr

TODO:

vjpr commented 6 years ago

Awesome!!

zkochan commented 6 years ago

After this change, implementing npm aliases will be easy. But I want to add more tests first.

Also, I still don't know what to do with bins. Should the default bin be renamed to the alias?

vjpr commented 6 years ago

Also, I still don't know what to do with bins. Should the default bin be renamed to the alias?

Yeh that would be cool. What does yarn do?

zkochan commented 6 years ago

As of version 1, Yarn keeps the original bin name. Renaming seems more logical to me, though.

Also, I suggest highlighting somehow in the summary the aliased dependencies.

Something like:

devDependencies:
+ mocha2 (realname: mocha) 2.0.0
+ mocha 4.0.0
zkochan commented 6 years ago

This might be considered a breaking change. I am not sure. Projects that will work with aliases won't work with older versions of pnpm. shrinkwrap.yaml files with aliases will break pnpm v1

andreineculau commented 6 years ago

I'm on a small screen and can't see the pr diff, but reading through the TODO - i would very much prefer if all the bins would just be prefixed (or actually suffixed since it would be more useful with tab completion in the shell), no special case for the default one.

Nicely done!

zkochan commented 6 years ago

Why no changes to the default one? My concern is that if you install two versions of the same package, there'll be a conflict. So if you install mocha@1 and mocha@2, which mocha will be in node_modules/.bin?

For the non-default ones, I guess we can use the prefix/suffix which was used to name the alias. So it'll work if mocha will have an alias called mocha2 or the-mocha. Otherwise, if the name is completely different, maybe the alias can be used as suffix. Also, what to do with special characters? Probably just strip them

andreineculau commented 6 years ago

I didn't say "no changes", just don't treat it special: rename the default bin, but pre/suffix the others. Pre/suffix all e.g. node_modules/.bin/mocha{,-} : mocha and mocha-mocha2. @ would be a natural separator but i guess it may cause confusion, if not also problems.

Since it's the developer deciding the alias, why be "smart" (pejorative) and strip "special" (who decides and based on what?) characters?

If the OS/file-system can create a file with that name, then just do it. Also for what we know now, most cases will just have alphanumeric dots hyphens (academic guessing given what the npm packages are named).

It's never nice neither to be surprised by how in some cases your expectations get validated, but not in others, nor to see opinionated software for no reason (i want an executable with a hyphen but can't because someone decide the hyphen is special, unsafe in 0.000001 cases etc)

zkochan commented 6 years ago

OK, I agree with not stripping special chars.

Suffixing the non-default bins with the full alias name is fine by me. I guess the - or _ should be used between the original command and the suffix.

A special case is scoped packages or alias names that contain the path separator symbol (/ or \). When a scoped package's default bin is linked into node_modules/.bin, only the last part of the name is used. So if you install @zkochan/pnpm, pnpm will be linked. I think this approach can be used for any aliases that contain the path separator symbol

andreineculau commented 6 years ago

Is it possible to warn/fail the installation if there's a conflict with namings? Or how does npm do if pkg a and b both have a binary c?

zkochan commented 6 years ago

npm just picks one. I don't know what the logic behind it is. Might be random.

It should be possible to add a warning about it but I don't think it should be part of this PR. Bins can conflict already, even w/o aliasing.

zkochan commented 6 years ago

After thinking about it more. This adds complexity and we're not even sure if the bins collision is an issue. Yarn has aliases and doesn't try to solve these...

There are too many scenarios.

For now I revert my changes to bins. Aliases won't change the bin names. We can add that in a future version.