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

refactor: introduce internal bound context #397

Merged
merged 20 commits into from May 26, 2022

Conversation

antongolub
Copy link
Contributor

@antongolub antongolub commented Apr 23, 2022

Introduces bound context to pass it down to helpers (verbose, quiet, timeout, etc).

Later we can add smth like:

$.o({spawn: customSpawn, quoute: v => v, nothrow: true})`cmd ${arg}`
$.quiet = $.o({verbose: false})
  • Tests pass

@antonmedv
Copy link
Collaborator

I didn’t get it) What problem does it solve? Everything seems more complicated with this.

@antongolub
Copy link
Contributor Author

For example, bound ctx lets us to configure logger params just once at the top level and do not pass this options directly from fn to fn to fn. I wanted to split it into several commits, but it might be better to add everything at once. Stay tuned.

@antonmedv
Copy link
Collaborator

I still now get it.

@antongolub
Copy link
Contributor Author

antongolub commented Apr 23, 2022

AsyncLocalStorage is pure magic!

Here's what we can end up with:

// run with default settings
await $`cmd `
await $`cmd1 ...`

// all nested are verbosed
await profile({verbose: false}, async () => {
   await $`cmd`
   await $`cmd`
   await $`cmd`
})

// all nested dont throw
await profile({nothrow: true, spawn: mockedSpawn}, async () => {
   await $`cmd`
   await $`cmd`
   await $`cmd`
   
   await profile({quote: customQuote, retry: 5}, async () => {
        // customQuotes, noThrow, mocked spawn and 5 attempts for each
       await $`cmd`
       await $`cmd`
   })
})
// regular call
await $`cmd`

@antongolub
Copy link
Contributor Author

antongolub commented Apr 23, 2022

With this improvement we can also provide a convenient testing API. As a separate pkg maybe.

@antonmedv
Copy link
Collaborator

But do we really need it? Testing right now works as well.

@antonmedv
Copy link
Collaborator

I like goods idea though

@antongolub
Copy link
Contributor Author

I can continue pushing commits here, but it will be hard to review. Should I add contextify API as part of this PR?

@antonmedv
Copy link
Collaborator

I’m not convinced what this API should be provided.

@antongolub
Copy link
Contributor Author

antongolub commented Apr 23, 2022

It's ok. But let's introduce als ctx, so we can add contextify as an extension on our own side.

@antonmedv
Copy link
Collaborator

But why? I still not understand. What problem does it solve?

@antongolub
Copy link
Contributor Author

antongolub commented Apr 23, 2022

  1. In the curr impl If we need to pass opts to $ factory we have to mutate its fields and then roll back.
$.spawn = spawn
const custom = $`cmd`
$.spawn = default

For both builtins or custom $-wraps.

  1. To extract and reuse helpers like printCmd, printStd, fetch.
  2. To bind log config when creating ProcessPromise instance and apply these settings down to the flow. (I think custom logger may appear in the near future)
  3. To decouple $ factory and ProcessPromise
  4. For bulk tweak ups.
// before
const $1 = (...args) => {$.smth = 'a'; const p = $(...args); $.smth = ''; return p}
const $2 = (...args) => {...}
const $2 = (...args) => {...}

// after
await profile({extra: 'npm_config_yes=true;'}, async () => {
   const ctx = getCtx()
   await $`cmd`
   await $`cmd`
   await $`${extra} cmd`
   await $`${extra} cmd`
})
  1. To implement tracing.
  2. To reduce complexity.
  3. To pass cwd w/o cd

Meaningful changes:
1.

import {AsyncLocalStorage} from "node:async_hooks";

export const als = new AsyncLocalStorage()
export const boundCtx = Symbol('AsyncLocalStorage bound ctx')
export const getCtx = () => als.getStore()
  1. $.somethinggetCtx().something
  2. $ itself is a root lvl ctx:
als.enterWith($)

@antongolub
Copy link
Contributor Author

antongolub commented Apr 23, 2022

@antonmedv

https://www.youtube.com/watch?v=eTAxFWzc1n4&t=5s

we use zx to operate monorepos, so we need to perform cwd/env-bound cmd queues — for each workspace in parallel

@antongolub
Copy link
Contributor Author

antongolub commented Apr 24, 2022

async_hooks is a modern native alternative to cls-hooked. In our experience it doesnt significantly affect performance.

@antongolub
Copy link
Contributor Author

antongolub commented Apr 25, 2022

async_hooks here is a little pinch of ioc/di, that significantly expands the customization options. I haven't refactored the tests to prove that the existing API contract hasn't changed in any way. Backward compatibility is not broken.

@antongolub
Copy link
Contributor Author

Give a chance to this API, and I'll show the true power of improvement in next pull requests.

@antonmedv
Copy link
Collaborator

Researching this right now.

What about

asyncLocalStorage.enterWith(store)#

Added in: v13.11.0, v12.17.0
Stability: 1 - ExperimentalasyncLocalStorage.enterWith(store)#

Added in: v13.11.0, v12.17.0
Stability: 1 - Experimental

@antongolub
Copy link
Contributor Author

antongolub commented Apr 25, 2022

Api details may be changed. But I don't remember any case when the added API was withdrawn at all (except punycode). async_hooks are intended for building large enterprise apps — for tracing, resource consumption measurements, low coupled configuration, etc. So I have no worries about its future.

In the worst case:

  1. cls / cls-hooked
  2. reset getCtx()back to $

@antongolub
Copy link
Contributor Author

npm publish beta --no-git-tag-version?

@antonmedv
Copy link
Collaborator

I need little time to review a bit.

@antonmedv
Copy link
Collaborator

Please, rebase on master.

src/core.mjs Outdated Show resolved Hide resolved
src/core.mjs Outdated Show resolved Hide resolved
src/util.mjs Outdated Show resolved Hide resolved
src/util.mjs Outdated Show resolved Hide resolved
test/index.test.mjs Show resolved Hide resolved
src/core.mjs Outdated Show resolved Hide resolved
src/als.mjs Outdated Show resolved Hide resolved
src/als.mjs Outdated Show resolved Hide resolved
src/goods.mjs Show resolved Hide resolved
src/als.mjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/core.mjs Outdated Show resolved Hide resolved
src/als.mjs Outdated Show resolved Hide resolved
src/util.mjs Outdated Show resolved Hide resolved
src/goods.mjs Outdated Show resolved Hide resolved
src/core.mjs Outdated Show resolved Hide resolved
@antonmedv antonmedv merged commit a741743 into google:main May 26, 2022
@antonmedv
Copy link
Collaborator

Congratulations 🎉 =))

@antongolub antongolub deleted the add-bound-ctx branch May 26, 2022 20:43
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