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

Run buildAll in prepare, move husky install to postinstall #1583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schleyfox
Copy link
Contributor

This is part of a series of PRs based on performance work we have done to
improve a use-case involving parsing/formatting hundreds of thousands of dates
where luxon was the bottleneck.

This is just an improvement to make it possible to use luxon as a git dependency (for instance when you're testing out a bunch of hopefully good changes on your fork).

Husky adds git hooks into the repo. We want the dev experience of

$ git clone remote/luxon
$ npm install # installs husky hooks
$ ...develop develop develop...
$ git commit
_lint runs, everything works_

We also want to build luxon when we call npm pack or npm publish.

We also want to build luxon when it is installed somewhere else as a git dependency
(https://docs.npmjs.com/cli/v10/configuring-npm/package-json#git-urls-as-dependencies)

Git dependencies work by running npm install from GitFetcher and then depend on DirFetcher running npm prepare (but only prepare, not prepack or postpack)

This change moves husky install to postinstall (which will, unfortunately, still pointlessly run when we install luxon as a git dependency). It moves the buildAll to prepare from prepack so that the build happens when installed as a git dependency. This should preserve existing behavior while enabling installation from git.

When installing from git and/or in CI environments, the husky command is sometimes not available in postinstall or is available but bails out when not run in the context of a git repo.

Husky adds git hooks into the repo. We want the dev experience of

    $ git clone remote/luxon
    $ npm install # installs husky hooks
    $ ...develop develop develop...
    $ git commit
    _lint runs, everything works_

We also want to build luxon when we call npm pack or npm publish.

We also want to build luxon when it is installed somewhere else as a git
dependency
(https://docs.npmjs.com/cli/v10/configuring-npm/package-json#git-urls-as-dependencies)

Git dependencies work by running `npm install` from GitFetcher and then
depend on DirFetcher running `npm prepare` (but only prepare, not
prepack or postpack)

This change moves `husky install` to postinstall (which will,
unfortunately, still pointlessly run when we install luxon as a git
dependency). It moves the buildAll to prepare from prepack so that the
build happens when installed as a git dependency. This should preserve
existing behavior while enabling installation from git.

When installing from git and/or in CI environments, the husky command is
sometimes not available in postinstall or is available but bails out
when not run in the context of a git repo.
Copy link

linux-foundation-easycla bot commented Jan 22, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: schleyfox / name: Ben Hughes (87fd3ff)

@schleyfox schleyfox marked this pull request as ready for review January 22, 2024 16:50
@schleyfox
Copy link
Contributor Author

/easycla

@icambron
Copy link
Member

This change moves husky install to postinstall (which will, unfortunately, still pointlessly run when we install luxon as a git dependency).

I may just not understand how these things work, but I'd have guessed postinstall would run even when it's a regular, non-git npm dependency, which doesn't seem great.

@schleyfox
Copy link
Contributor Author

yeah, that does seem possible, I'll test some more to see what's up. The docs around lifecycle scripts are confusing and, I strongly suspect, inaccurate (and the code isn't straightforward either). I'll test more and find a hook that works.

FWIW the docs

[npm install](https://docs.npmjs.com/cli/v9/commands/npm-install)
These also run when you run npm install -g <pkg-name>

preinstall
install
postinstall
prepublish
preprepare
prepare
postprepare

@schleyfox
Copy link
Contributor Author

Finally got around to testing this, results in https://github.com/schleyfox/schleyfox-npm-lifecycle-test

npm install in the package dir (for e.g. development) runs

2024-05-03T15:11:39+0000 preinstall
2024-05-03T15:11:39+0000 install
2024-05-03T15:11:39+0000 postinstall
2024-05-03T15:11:39+0000 prepublish
2024-05-03T15:11:39+0000 preprepare
2024-05-03T15:11:39+0000 prepare
2024-05-03T15:11:39+0000 postprepare

while npm install in another app pulling the dependency from npm runs

2024-05-03T15:11:40+0000 preinstall
2024-05-03T15:11:40+0000 install
2024-05-03T15:11:40+0000 postinstall

install of a git dependency runs

2024-05-03T15:11:41+0000 preinstall
2024-05-03T15:11:41+0000 install
2024-05-03T15:11:41+0000 postinstall
2024-05-03T15:11:41+0000 prepublish
2024-05-03T15:11:41+0000 preprepare
2024-05-03T15:11:41+0000 prepare
2024-05-03T15:11:41+0000 postprepare
2024-05-03T15:11:41+0000 prepare
2024-05-03T15:11:41+0000 preinstall
2024-05-03T15:11:41+0000 install
2024-05-03T15:11:41+0000 postinstall

(I think that is an install (which includes prepare) followed by a bare prepare (no pre- or post-) followed by an npm-install-as-a-dependency (with pre- and post-)

npm publish runs

2024-05-03T10:26:20-0400 prepublishOnly
2024-05-03T10:26:20-0400 prepack
2024-05-03T10:26:20-0400 prepare
2024-05-03T10:26:20-0400 postpack
2024-05-03T10:26:50-0400 publish
2024-05-03T10:26:50-0400 postpublish

I think this indicates that prepare is the right place for husky to run, but it's also the right place to do the build. I'll update the prepare script to babel-node tasks/buildAll.js && (husky install || true)

@icambron
Copy link
Member

icambron commented Aug 3, 2024

@schleyfox i sort of lost track of this one -- looks like there might be one more todo here?

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.

2 participants