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.

Sign up for GitHub

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

Jump to bottom

Always put brotli payload in execArgv, even for non-esm bootstrapping #1831

Open
Tracked by #1505
cspotcode opened this issue Jul 10, 2022 · 0 comments
Open
Tracked by #1505

Always put brotli payload in execArgv, even for non-esm bootstrapping #1831

cspotcode opened this issue Jul 10, 2022 · 0 comments
Labels
research Needs design work, investigation, or prototyping. Implementation uncertain.
Milestone
v11

Comments

@cspotcode
Copy link
Collaborator

cspotcode commented Jul 10, 2022 •
edited
Loading

Right now, if you fork() a ts-node process, the child gets the ts-node bin entrypoint and the same flags passed to ts-node: --project, etc. Yet if you've launched with --esm, then the child receives a pre-packed brotli payload of parsed configuration & bootstrapping information.

Without brotli, child process re-loads tsconfig, re-does all config work
With brotli, some / most of config loading is already done and baked into the brotli payload

Should these 2 codepaths be unified? So that we always put the brotli payload in execArgv?

Once we have fully completed bootstrapping and config parsing, we can re-pack the brotli payload and put it in execArgv.

Pros

  • prevents subsequent child processes from repeating any of that config work
    • skip loading certain deps
    • skip loading TS compiler entirely if using SWC?
  • children are forced to use the same tsconfig and flags even if they're spawned in a different working directory
    • probably a good thing for simplicity
    • opens the door for future work where children talk to the same language service in external process perhaps?

Cons

  • breaks rare cases where child_processes are expected to re-discover tsconfig
    • these cases can be accomplished by scrubbing the execArgv / re-spawning ts-node CLI using cross-spawn or similar

Other notes

Will only happen in our bin.ts bootstrapping; not --loader nor register() codepaths, since the latter do not control the node process' startup.
@cspotcode cspotcode added the research Needs design work, investigation, or prototyping. Implementation uncertain. label Jul 10, 2022
@cspotcode cspotcode added this to the v11 breaking changes milestone Jul 10, 2022
This was referenced Jul 10, 2022
Open
Merged
devversion added a commit to devversion/ts-node that referenced this issue Jul 13, 2022
@devversion
Fix ESM node processes being unable to fork into other scripts
7c0678f 
Currently, Node processes instantiated through the `--esm` flag result
in a child process being created so that the ESM loader can be
registered. This works fine and is reasonable.

The child process approach to register ESM hooks currently prevents
the NodeJS `fork` method from being used because the `execArgv`
propagated into forked processes causes `ts-node` (which is also
propagated as child exec script -- this is good because it allows nested
type resolution to work) to always execute the original entry-point,
causing potential infinite loops because the designated fork module
script is not executed as expected.

This commit fixes this by not encoding the entry-point information into
the state that is captured as part of the `execArgv`. Instead the
entry-point information is always retrieved from the parsed rest command
line arguments in the final stage (`phase4`).

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
@devversion devversion mentioned this issue Jul 13, 2022
Closed
devversion added a commit to devversion/ts-node that referenced this issue Jul 13, 2022
@devversion
Fix ESM node processes being unable to fork into other scripts
3812d56 
Currently, Node processes instantiated through the `--esm` flag result
in a child process being created so that the ESM loader can be
registered. This works fine and is reasonable.

The child process approach to register ESM hooks currently prevents
the NodeJS `fork` method from being used because the `execArgv`
propagated into forked processes causes `ts-node` (which is also
propagated as child exec script -- this is good because it allows nested
type resolution to work) to always execute the original entry-point,
causing potential infinite loops because the designated fork module
script is not executed as expected.

This commit fixes this by not encoding the entry-point information into
the state that is captured as part of the `execArgv`. Instead the
entry-point information is always retrieved from the parsed rest command
line arguments in the final stage (`phase4`).

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Fix ESM node processes being unable to fork into other scripts
4c3390a 
Currently, Node processes instantiated through the `--esm` flag result
in a child process being created so that the ESM loader can be
registered. This works fine and is reasonable.

The child process approach to register ESM hooks currently prevents
the NodeJS `fork` method from being used because the `execArgv`
propagated into forked processes causes `ts-node` (which is also
propagated as child exec script -- this is good because it allows nested
type resolution to work) to always execute the original entry-point,
causing potential infinite loops because the designated fork module
script is not executed as expected.

This commit fixes this by not encoding the entry-point information into
the state that is captured as part of the `execArgv`. Instead the
entry-point information is always retrieved from the parsed rest command
line arguments in the final stage (`phase4`).

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Fix ESM node processes being unable to fork into other scripts
f609518 
Currently, Node processes instantiated through the `--esm` flag result
in a child process being created so that the ESM loader can be
registered. This works fine and is reasonable.

The child process approach to register ESM hooks currently prevents
the NodeJS `fork` method from being used because the `execArgv`
propagated into forked processes causes `ts-node` (which is also
propagated as child exec script -- this is good because it allows nested
type resolution to work) to always execute the original entry-point,
causing potential infinite loops because the designated fork module
script is not executed as expected.

This commit fixes this by not encoding the entry-point information into
the state that is captured as part of the `execArgv`. Instead the
entry-point information is always retrieved from the parsed rest command
line arguments in the final stage (`phase4`).

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Consistently perform bootstrap and encode Brotli config for improved …
a7c46a4 
…caching/reduced complexity

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Consistently perform bootstrap and encode Brotli config for improved …
cbdf2c2 
…caching/reduced complexity

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Consistently perform bootstrap and encode Brotli config for improved …
4232900 
…caching/reduced complexity

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Consistently perform bootstrap and encode Brotli config for improved …
a7e6486 
…caching/reduced complexity

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Consistently perform bootstrap and encode Brotli config for improved …
6f0ee70 
…caching/reduced complexity

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
@devversion
Consistently perform bootstrap and encode Brotli config for improved …
ee025e8 
…caching/reduced complexity

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
research Needs design work, investigation, or prototyping. Implementation uncertain.
Projects
None yet
Milestone
v11
Development

No branches or pull requests
1 participant
@cspotcode