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

Add postinstall script to readme #65

Merged
merged 2 commits into from
Jan 29, 2021
Merged

Add postinstall script to readme #65

merged 2 commits into from
Jan 29, 2021

Conversation

karlhorky
Copy link
Contributor

Closes #63

@coveralls
Copy link
Collaborator

Coverage Status

Coverage remained the same at 100.0% when pulling e649fa5 on karlhorky:patch-1 into c1f4630 on jeffijoe:master.

@jeffijoe jeffijoe merged commit a623e61 into jeffijoe:master Jan 29, 2021
@jeffijoe
Copy link
Owner

Thanks!

@karlhorky
Copy link
Contributor Author

Glad to help, thanks for the merge!

@karlhorky karlhorky deleted the patch-1 branch January 29, 2021 17:13
@mike-clark-8192
Copy link

This might be obvious to experienced npm users, but it wasn't to me: postinstall scripts in your project's main package.json don't run after installing specific packages (e.g. npm install lodash). They only run when you do an unqualified npm install.

After some research, I found there is no good solution to this. The only way to get something to run after npm install [package] is to use a hook script, and hook scripts are platform dependent, and a mess to setup.

@karlhorky
Copy link
Contributor Author

Good point, sorry about this. I looked around a bit, and it seems this is known behavior of npm (if unwanted).

Yarn does it correctly (runs postinstall on every yarn install <pkg>), so I didn't think that npm would have such weird, different behvior. I've asked if they are willing to change this: npm/feedback#217

Since it would be a while before any change, and npm users are the majority (unfortunately can't just write a guide that works for Yarn), I've opened #66 with a shell script alternative:

npm run i <pkg>

Even nicer if typesync could take a package name as an argument:

typesync install <pkg name>

or just:

typesync <pkg name>

and then it would handle the installation, like typac does. Then there would be no need for a shell script.

@mike-clark-8192
Copy link

mike-clark-8192 commented Feb 17, 2021

karlhorky, I agree with your assessment of the automation scripting. If only everyone could switch to yarn ...

Regarding typesync install <package>, I would definitely use this feature. Currently I use typedi to install new packages and typesync to download+install types for a pre-dependencies already in package.json. I like to use just one tool, and I prefer typesync! Of course, you can always npm i [package] followed by typesync, but you know picky developers are about saving keystrokes and not wanting to have to write their own aliases/wrappers. :]

@jeffijoe
Copy link
Owner

@mike-clark-8192 that workflow sounds like typesync is only used once, and typedi going forward. It sounds like typedi solves the problem then?

@mike-clark-8192
Copy link

mike-clark-8192 commented Feb 17, 2021

Use-cases for typesync (npm assumed):

  1. Install typedefs into an existing project that has untyped dependencies:
    $ typesync
    $ npm i
  2. Synchronize the typedefs for an existing "typed" project that has untyped dependencies that are out of sync with their typedefs:
    $ typesync
    $ npm i
  3. Install a new untyped library and its typedefs:
    $ npm i lodash
    $ typesync
    $ npm i

The dream of the package.json "postinstall" script was that you could simplify all three of these scenarios (assuming you had added the the postinstall script to your project's package.json ahead of time):

  1. Install typedefs into an existing project that has untyped dependencies
    $ npm i
  2. Synchronize the typedefs for an existing "typed" project that has untyped dependencies that are out of sync with their typedefs:
    $ npm i
  3. Install a new untyped library and its typedefs:
    $ npm i lodash

I believe all three work under yarn(? I'm not a yarn guy, haven't tested), but the third case, which offers the most savings, doesn't work under npm because npm doesn't run postinstall after installing a new package. Also, the npm devs have said they never will change this 'quirk' (their term) of postinstall.

If you want to ignore the postinstall idea altogether and mix in the usage of typedi, you get this scenario, which works OK:

  1. Install typedefs into an existing project that has untyped dependencies:
    $ typesync
    $ npm i
  2. Synchronize the typedefs for an existing "typed" project that has untyped dependencies that are out of sync with their typedefs:
    $ typesync
    $ npm i
  3. Install a new untyped library and its typedefs:
    $ typedi lodash

However, I think there was some hope that maybe the typesync maintainers might be interested in implementing everything a lazy njs/npm/ts dev could ever want :):

  1. Install typedefs into an existing project that has untyped dependencies:
    $ typesync i
  2. Synchronize the typedefs for an existing "typed" project that has untyped dependencies that are out of sync with their typedefs:
    $ typesync i
  3. Install a new untyped library and its typedefs:
    $ typesync i lodash

I might actually look into this myself some day, but I have several weeks of work piled up, so I'll check back in when I free up. Thanks!

@jeffijoe
Copy link
Owner

If I were to add this, would the install be carried out by shelling out to npm or yarn?

My concern is having to parse arguments like --saveDev, -D and others.

@mike-clark-8192
Copy link

mike-clark-8192 commented Feb 17, 2021

If I were to add this, would the install be carried out by shelling out to npm or yarn?

Ha! Good catch. I hadn't thought of that. I'm not gonna lie, this will be a little messy to implement, but I think users will love it. I'll type up some notes and reply in a bit.

@mike-clark-8192
Copy link

mike-clark-8192 commented Feb 18, 2021

Additional CLI user interface

typesync <i|install> [options] <package> [...packages]
    Install package(s) and their external typedefs. If a package does not 
    have external typedefs available, the packages will still be installed.

    This command is equivalent to running (e.g. with npm):
        npm install [options] <package> [...packages]
        typesync
        npm install

    Options:
        -D, --save-dev              install to devDependencies
        -P, --save-prod             install to dependencies
        -O, --save-optional         install to optionalDependencies
        -m, --manager [npm|yarn]    force preferred package manager

typesync <i|install> [options]
    Installs and syncs external typedefs for all of your package.json's
    dependencies that have external typedefs available.
    This command is equivalent to running (e.g. with npm):
        typesync
        npm install

    By default your preferred package manager will be guessed by the the
    presence one of the following files:
       File                          Package Manager
        yarn.lock                      yarn
        package-lock.json              npm
        pnpm-lock.yaml                 pnpm (not yet supported)

    If typesync can't guess your package manager, it will exit with an error. You 
    can resolve this situation by removing unused lock files, or use -m as below:

    Options:
        -m, --manager [npm|yarn]    force preferred package manager

Implementation notes

Invoking the package manager

See the cli --help above for info on guessing the user's package manager (and the ability to override the guess).

NPM (and probably yarn) programmatic APIs aren't stable. Their CLI is their stable interface, so child_process.exec is probably the only way to do this that isn't an impossible versioning nightmare. We have to programmatically construct the correct command to issue to the PM, but this is relatively easy.

Relaying package manager output back to the user

We must capture stdout and stderr from exec(), then write it back to typesync's stderr + stdout. We don't need to interpret/parse the output because the process exit code (available from exec) will tell us success vs. failure, which is all we need to branch on.

Crazy npm arg parsing and infrequently used options

  1. We don't need to support all the crazy possible allowed invocations of npm + install. NPM allows you put flags anywhere and has typo correction and all sorts of silly unnecessary things. Example valid command, which actually works (and not incorrectly):
    npm -D isntall --save-bundle lodash --dry-run Huh? No thanks.
  2. Only need to support the basic grammar outlined in the --help I wrote above.
  3. Don't need to support all the PM flags. typesync i is a convenience thing. If a user needs full control over the package manager invocation, they can continue to use the traditional invocation of typesync, which won't go away.

The typed-install project

The typed-install (typedi) project (https://www.npmjs.com/package/typed-install) already implements the most of the "new" code that would be needed for the above. I wonder if the maintainer of that project is interested in merging functionality under the heading of one tool? Or letting typesync borrow some code? Or vice-versa?

Does typesync even need it?

typesync is fully functional as it is right now, and maybe the extra bit of convenience added by the above (plus the presence of already-existing complimentary tools) isn't worth complicating the project with. Anyway, it was fun to talk about, so no loss if it isn't a good fit.

Cheers! :)

@jeffijoe
Copy link
Owner

Thanks for the detailed write-up!

I don't currently have a lot of free time to work on these things, however. And I guess I just don't install new packages often enough for this to bother me. 😅

@karlhorky
Copy link
Contributor Author

Also, the npm devs have said they never will change this 'quirk' (their term) of postinstall.

There has been a bit of movement on this point actually - I submitted an npm RFC and the npm maintainers commented that there is a valid use case there and that they want to do something about this (maybe even a new feature with a new name that allows for this workflow).

@karlhorky
Copy link
Contributor Author

karlhorky commented Feb 18, 2021

If I were to add this, would the install be carried out by shelling out to npm or yarn?

My concern is having to parse arguments like --saveDev, -D and others.

this will be a little messy to implement

I think there would be a simple way to do it, if @jeffijoe would be up for it.

Don't worry about all of the arguments - keeping up with the package managers is not going to be a good time.

Just pass all of them over to the package manager and let it handle them (the user should know which arguments they can use and how). One possible exception - a --yarn flag (see below).

So eg.

typesync install --save-dev styled-components

would just get translated to:

npm install --save-dev styled-components @types/styled-components

Or, if they use Yarn:

typesync install --yarn --dev styled-components

would just get translated to:

yarn add --dev styled-components @types/styled-components

One special feature (but also pretty simple to implement) would be to install @types/ packages to devDependencies by default, so that this command:

typesync install styled-components

becomes:

npm install styled-components
npm install --save-dev styled-components

@mike-clark-8192
Copy link

mike-clark-8192 commented Feb 18, 2021

@karlhorky it sounds like jeffijoe has enough work on his plate without having to worry about integrating with package mangers via exec(). Entirely from an end-user's perspective, I feel the feature is a natural fit for what some end-users want to get out of typesync. However, from a technical perspective, the package-manager integration "subsystem" is an entirely different beast from typesync's core functionality -- which is syncing types in the package descriptor.

I think I might try to prototype a meta-module that integrates the functionality of typesync and typedi into a single seamless CLI interface. I'll invite you to the repo when I set it up!

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.

Add postinstall script recommendation in readme
4 participants