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

winterqt/libnpmpack foreground scripts #5645

Merged
merged 3 commits into from Oct 5, 2022

Conversation

wraithgar
Copy link
Member

  • fix(libnpmpack): obey foregroundScripts
  • chore: add spawk as a dev dependency

@wraithgar wraithgar requested a review from a team as a code owner October 5, 2022 17:41
@wraithgar
Copy link
Member Author

This is building off of the work already completed in #5430.

All this is doing is installing spawk correctly so that all of the hard work that @winterqt has done can be shipped.

@wraithgar wraithgar force-pushed the winterqt/libnpmpack-foreground-scripts branch from 559b6a7 to a12bb39 Compare October 5, 2022 18:21
@wraithgar wraithgar merged commit 9f8e3f0 into latest Oct 5, 2022
@wraithgar wraithgar deleted the winterqt/libnpmpack-foreground-scripts branch October 5, 2022 18:32
@winterqt
Copy link
Contributor

winterqt commented Oct 5, 2022

Doesn't initializing spawk in the test like that cause child_process.spawn to not be overridden in pack, since it's imported before overriding it?

@wraithgar
Copy link
Member Author

Nope, child_process is a singleton, it's the exact same object wherever it's included. So as long as things aren't de-referencing the spawn function itself off child_process, it will get the mocked version whenever it runs child_process.spawn.

@wraithgar
Copy link
Member Author

Oh, well I see what you mean here https://github.com/npm/promise-spawn/blob/main/lib/index.js#L1

The prepack.called is being asserted in the tests so it is working ... Lemme see if I can be sure as to why.

@wraithgar
Copy link
Member Author

Oh I mixed tspawk and tnock up when reading the code. There's no mystery here. const tspawk = require('./fixtures/tspawk.js') happens right off the bat. It loads itself in sync as it is required. Everything after that that asks for child_process.spawn gets spawk. The only "quirk" here would be if somehow the code were to be called again after tap ran teardown, it would still be spawk that it was getting, not the real child_process.spawn.

TLDR when using spawk make sure it's required before anything else that will require child_process.spawn

@winterqt
Copy link
Contributor

winterqt commented Oct 6, 2022

Ah, right, you loaded it before everything else but only initialized it further in the tests.

Was the change from npm.prefix to testDir in makeSpawnArgs the change that fixed the rmdir failure on Windows? Or was it just a race condition or something? Seems weird.

@wraithgar
Copy link
Member Author

The rmdir error was because of npm being used at all. The workspace tests don't instantiate npm, they test the library themselves. When npm was being initialized in tests, but nothing about it was awaited, there were logs happening asynchronously that were being written to after the test had finished, and the windows cleanup methods threw an exception in that case.

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.

None yet

2 participants