8000 Misleading documentation · Issue #876 · google/zx · GitHub
[go: up one dir, main page]
Skip to content

Misleading documentation #876

New issue
New issue
Open
#1536
Open
Misleading documentation#876
#1536
Labels
docs needed

Description

@gear4s
gear4s
opened on Aug 23, 2024

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

Activity

  1. antonmedv commented on Aug 23, 2024

    @antonmedv
    antonmedv
    on Aug 23, 2024
    Collaborator

    @antongolub please take a look, thanks.

  2. antongolub commented on Aug 25, 2024

    @antongolub
    antongolub
    on Aug 25, 2024
    Collaborator

    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()

  3. gear4s commented on Aug 25, 2024

    @gear4s
    gear4s
    on Aug 25, 2024
    Author

    @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

  4. antongolub commented on Aug 26, 2024

    @antongolub
    antongolub
    on Aug 26, 2024
    Collaborator

    @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

  5. antongolub commented on Aug 27, 2024

    @antongolub
    antongolub
    on Aug 27, 2024
    Collaborator

    @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?

  6. gear4s commented on Aug 27, 2024

    @gear4s
    gear4s
    on Aug 27, 2024
    Author

    @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`);
});

  7. antonmedv commented on Sep 14, 2024

    @antonmedv
    antonmedv
    on Sep 14, 2024
    Collaborator

    So do we need to update some docs?

  8. antonmedv
    added
    docs needed
    on Sep 14, 2024

  9. antongolub commented on Sep 15, 2024

    @antongolub
    antongolub
    on Sep 15, 2024
    Collaborator

    We have a notice: https://google.github.io/zx/api#cd Should additional description be provided?

  10. antonmedv commented on Sep 16, 2024

    @antonmedv
    antonmedv
    on Sep 16, 2024
    Collaborator

    Let's add more documentation to https://google.github.io/zx/api#syncprocesscwd

    More examples and explain what is the difference in terms of async contexts.

    For example, without syncprocesscwd a different callback maybe out of sync.

  11. gear4s commented on Sep 21, 2024

    @gear4s
    gear4s
    on Sep 21, 2024
    Author

    after doing some research into async hooks, do you guys think there could be another method of tracking cd in separate within contexts? using something like https://github.com/laverdet/isolated-vm or https://github.com/avoidwork/tiny-worker

  12. antonmedv commented on Sep 22, 2024

    @antonmedv
    antonmedv
    on Sep 22, 2024
    Collaborator

    Tiny-worker just spawns a process with different node script. This will not help here.

    Isolates are cool. Bur also not applicable here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    docs needed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions
      0