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

feat: Add prefetch script to install and ci commands #5264

Closed
wants to merge 1 commit into from

Conversation

bearfriend
Copy link

@bearfriend bearfriend commented Aug 4, 2022

In v7 the preinstall script was intentionally changed to run after dependencies are installed. This was never documented as a breaking change and has since then been neither fixed nor otherwise remedied. While I personally would argue that since it was never listed as a breaking change it should be considered a bug, I recognize that the time elapsed since then complicates matters.

Because of that, I propose simply adding a new script that will run before dependencies are installed to restore a critical piece of functionality that was removed without being documented. An oft-cited use for a script being run at that point is to authenticate to a private registry before attempting to fetch packages.

I also recognize this issue may be more complicated than my initial single commit attempts to make it seem, but the bug report (#2660) seems to have stagnated since it was submitted in February of 2021, and as far as I can tell no one has even attempted a fix. There is debate over whether this inaction means that it must be very complicated or not. I hope that this PR will at least spur additional debate. I'm happy to be proven wrong.

I'm largely unfamiliar with the npm codebase, so please do suggest a different script name (I chose prefetch fairly arbitrarily just based on my understanding, but have no attachment to it), different placement for the runScript call etc., or additional required changes/tests I may have missed. Or if someone with more understanding of the details is inspired to pick this up and run with it, by all means, please do so.

Edit: It looks like there was an early attempt (#2713) to move preinstall before reify that was closed because it would be "breaking" (even though, again, this would actually just fix an undocumented change), but no alternative remedy was ever acted on.

References

Fixes #2660
Related to #2713
Related to npm/rfcs#403

@bearfriend bearfriend requested a review from a team as a code owner August 4, 2022 21:42
@bearfriend bearfriend changed the title Add prefetch script to install and ci commands feat: Add prefetch script to install and ci commands Aug 4, 2022
@jay-p-b-7span
Copy link

Please fix the issue.

@MVMS1994
Copy link

when will this be merged?

@ljharb
Copy link
Collaborator

ljharb commented Aug 10, 2022

Changes like this need to be an RFC first.

@bearfriend
Copy link
Author

@ljharb It seems like there already is one to do nearly exactly the same thing (npm/rfcs#403), but nothing has been done on it in over a year. Is there something I can do to get this issue fixed?

@ljharb
Copy link
Collaborator

ljharb commented Aug 10, 2022

Nope. Looks like #2660 Is the issue to watch.

@bearfriend
Copy link
Author

bearfriend commented Aug 10, 2022

It would seem that the issue needs a kickstart. There has been no activity on that issue either. Not even an update on the plan. As I mentioned in my comment there, and here, this change in behavior was never documented and so it would seem to be a critical bug that is not being fixed.

An update of some kind would be greatly appreciated.

@wraithgar
Copy link
Member

As @ljharb stated, this kind of a change needs to go through the rfc process. I know it can be frustrating having to wait on changes like this but we are trying to be very cautious with the lifecycle scripts.

@wraithgar wraithgar closed this Nov 1, 2022
@bearfriend
Copy link
Author

As @ljharb stated, this kind of a change needs to go through the rfc process. I know it can be frustrating having to wait on changes like this but we are trying to be very cautious with the lifecycle scripts.

But, you were not careful with the lifecycle scripts. That's why this breaking change-bug exists. I apologize for being so direct but no one seems to be acknowledging this crucial point.

@wraithgar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Triage needs review for next steps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Preinstall script runs after installing dependencies
6 participants