google / zx

A tool for writing better scripts
https://google.github.io/zx/
Apache License 2.0
42.82k stars 1.09k forks source link

Misleading documentation #876

Open gear4s opened 2 weeks ago

gear4s commented 2 weeks ago

Expected Behavior

The documentation for within() does not use cd() due to it touching process.cwd and not reverting in some way, but having no mention of this side-effect as part of within()

Actual Behavior

The documentation for within() uses cd() which changes the process.cwd of the entire script and introduces parity between process.cwd and $`cd`

Steps to Reproduce

  1. Run this code for breaking example:
    
    echo("Script start");
    echo(`- ${await $`pwd`}`); // => cwd

await within(async () => { echo("First within"); cd("/"); echo(- ${await $pwd}); // => / });

echo("Between within"); echo(- ${await $pwd}); // => cwd

await within(async () => { echo("Second within"); echo(- process CWD: ${process.cwd()}); // => / echo(- ${await $pwd}); // => cwd });

echo("Script end"); echo(- ${await $pwd}); // => cwd



### Specifications

- zx version:  8.1.4
- Platform: macos
- Runtime: node
antonmedv commented 2 weeks ago

@antongolub please take a look, thanks.

antongolub commented 2 weeks ago

I see... cd() changes the global process.cwd, but internal $[processCwd] = process.cwd() assignment relates the local storage ctx only. So the root lvl $[processCwd] still keeps the initial default process.cwd() value. Previously we've used a sync hook to revert the cd side-effects for other within contexts, but since v8 is should be enabled manually via syncProcessCwd()

gear4s commented 2 weeks ago

@antongolub is there another API we could use to replace the context hook? seems like node upstream wants to remove it in its current capacity - maybe it should be reimplemented

antongolub commented 2 weeks ago

@gear4s, it looks as though it is going to be. Thanks for highlighting this. We'll keep that in mind.

@antonmedv, fyi.

https://nodejs.org/api/async_hooks.html#async-hooks

Stability: 1 - Experimental. Please migrate away from this API, if you can. We do not recommend using the createHook, AsyncHook, and executionAsyncResource APIs as they have usability issues, safety risks, and performance implications. Async context tracking use cases are better served by the stable AsyncLocalStorage API. If you have a use case for createHook, AsyncHook, or executionAsyncResource beyond the context tracking need solved by AsyncLocalStorage or diagnostics data currently provided by Diagnostics Chan

antongolub commented 2 weeks ago

@gear4s,

Could you tell us more about your practical example? Why do you need to switch the process dir in your case? Why not just $.pwd = '/' inside within callback?

gear4s commented 2 weeks ago

@gear4s,

Could you tell us more about your practical example?

I am building a tool to perform migrations of folders and files, and each migration has steps to execute. each step is executed in a within context. I am making pretty heavy use of fs.move within the within context, which only has context of process.cwd, which then results in Error: ENOENT: no such file or directory, open ....

effectively I am trying to do this

within(async () => {
  cd(`${basePath}/project/folder`);

  await $`sed -i '' 's|../path|../path2|' ./utils.js`;
  await $`sed -i '' 's|__dirname}/../../|__dirname}/../|' ./utils.js`;
  await $`sed -i '' 's|project/tests|tests|' ./folder/generator.sh`;
});

within(async () => {
  cd(basePath); // << failure happens here
  await fs.move(`./folder/folder`, `./otherFolder`);
  await fs.move(`./folder/folder2`, `./otherFolder2`);
});