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

fix(subs): disable save/kwargs mutation + better LOG_LEVEL #915

Merged
merged 6 commits into from
Nov 14, 2024

Conversation

michael-0acf4
Copy link
Contributor

@michael-0acf4 michael-0acf4 commented Nov 12, 2024

Tackles MET-724 and MET-730 .

  • A reference to run.kwargs should not be exposed directly, the kwargs used by the user should be a deep copy.
  • save now returns a deep copy of the returned value
  • noisy debug logs on substantial agent (disabled by default)

Migration notes

save will always refer to a deep clone of a value throughout the initial run/replay.

const example = await ctx.save(() => someRef);
someRef.field = "changed!"; // will not affect example
  • The change comes with new or modified tests
  • Hard-to-understand functions have explanatory comments
  • End-user documentation is updated to reflect the change

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 34 lines in your changes missing coverage. Please review.

Project coverage is 77.68%. Comparing base (7681e24) to head (bc46017).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/typegate/src/runtimes/substantial/agent.ts 60.93% 25 Missing ⚠️
src/typegate/src/log.ts 71.42% 8 Missing ⚠️
tests/runtimes/substantial/common.ts 98.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #915      +/-   ##
==========================================
+ Coverage   77.58%   77.68%   +0.10%     
==========================================
  Files         149      149              
  Lines       18385    18486     +101     
  Branches     1788     1793       +5     
==========================================
+ Hits        14264    14361      +97     
- Misses       4098     4102       +4     
  Partials       23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michael-0acf4 michael-0acf4 changed the title fix(subs): save/input kwargs affected by outside references issue + SUBSTANTIAL_TRACE fix(subs): save/kwargs mutation + better LOG_LEVEL Nov 13, 2024
@michael-0acf4 michael-0acf4 changed the title fix(subs): save/kwargs mutation + better LOG_LEVEL fix(subs): disable save/kwargs mutation + better LOG_LEVEL Nov 13, 2024
@michael-0acf4
Copy link
Contributor Author

published_test.ts seems to inherit the current env variables...

Signed-off-by: michael-0acf4 <[email protected]>
Natoandro
Natoandro previously approved these changes Nov 14, 2024
@michael-0acf4 michael-0acf4 dismissed stale reviews from Natoandro and Yohe-Am via bc46017 November 14, 2024 10:31
@michael-0acf4 michael-0acf4 merged commit f95c122 into main Nov 14, 2024
13 of 14 checks passed
@michael-0acf4 michael-0acf4 deleted the substantial-fixes branch November 14, 2024 14:15
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.

4 participants