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

Fixed: npm publish lifecycle order #2636

Closed
wants to merge 1 commit into from
Closed

Fixed: npm publish lifecycle order #2636

wants to merge 1 commit into from

Conversation

CxRes
Copy link

@CxRes CxRes commented Feb 5, 2021

The lifecycle order for npm publish was corrected to match actual behavior and no longer contradicts the description for prepare .

The lifecycle order for `npm publish` was corrected to match actual behavior and no longer contradicts the description for prepare .
@CxRes CxRes requested a review from a team as a code owner February 5, 2021 23:35
@CxRes
Copy link
Author

CxRes commented Feb 5, 2021

This change needs to be backported as well!

@wraithgar
Copy link
Member

We may need to take a slightly closer look at this, because unless I'm missing something prepublish isn't run during publish, and there are comments in both install and ci saying "should we remove this finally?" for prepublish.

@CxRes
Copy link
Author

CxRes commented Feb 7, 2021

@wraithgar Do something very simple. In your package.json:

{
  ...
  scripts: {
    ...
    "prepublish": "echo prepublish",
    "prepublishOnly": "echo prepublishOnly",
    "prepare": "echo prepare"
    }
}

and run:

npm publish . --dry-run 

@CxRes
Copy link
Author

CxRes commented Feb 7, 2021

prepublish is meant to behave at some point in the future like prepublishOnly and no longer like prepare. Thats for npm people to decide, but the documentation bug about behaviour today should be fixed now!

@wraithgar
Copy link
Member

wraithgar commented Feb 8, 2021

What version of npm are you using?

~/D/n/foo $ npm publish . --loglevel silent --dry-run

> [email protected] prepublishOnly
> echo prepublishOnly

prepublishOnly
~/D/n/foo $ npm -v
7.5.2
~/D/n/foo $ cat package.json|json scripts
{
  "prepublish": "echo prepublish",
  "prepublishOnly": "echo prepublishOnly",
  "prepare": "echo prepare"
}

@CxRes
Copy link
Author

CxRes commented Feb 8, 2021

@wraithgar I am on 6.14.10 (shipped with node 14.15.4). That explains the difference we are seeing, (except it doesn't solve the matter but actually makes things worse, see below)!

output with 6.14.10

prepublish
prepare
prepublishOnly

output with 7.5.2

prepublishOnly

Note that prepare not showing up either in v7. Have both prepare and prepublish been removed from the publish lifecycle? If this is correct behaviour, it is an undocumented breaking change.

@wraithgar
Copy link
Member

Prepare runs, but its output is suppressed, and it doesn't run during a dry-run.

We did a quick spike to investigate and document the lifecycle scripts, and the initial research is done. Now all that's left is getting the docs up to date to reflect reality, and reflect all of it.

npm/statusboard#267

@wraithgar
Copy link
Member

Saving you a click, here's what runs during publish

  • prepublishOnly
  • prepack
  • prepare (not during dry-run)
  • postpack
  • publish
  • postpublish

@CxRes
Copy link
Author

CxRes commented Feb 11, 2021

Just to confirm, in that order? because it is different from 6.x?

@wraithgar
Copy link
Member

Yes this is the order in which lifecycle events happen during publish in npm@7

@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release semver:patch semver patch level for changes labels Feb 11, 2021
@wraithgar
Copy link
Member

This change and many other is going to be reflected in #2690

@wraithgar wraithgar closed this Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants