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

Improve httpRegion dependency tracking #249

Merged
merged 4 commits into from
Apr 25, 2022

Conversation

fredrikhr
Copy link
Contributor

@fredrikhr fredrikhr commented Apr 5, 2022

A non-global httpRegion should implicitly be dependent on the global regions of the file.

Since global regions are always executed when @import-ing or directly executing a region in a file, it is not helpful to invalidate their dependents when global regions are executed. Since a global region cannot be @ref-ed in itself that is okay.

Having the dependency tracking on the context is not ideal: the context is recreated every time the httpyac-CLI or VS Code execute a region or file. Instead dependency tracking should really be a feature of the region itself.

This makes the context argument for the registerRegionDependent and resetDependentRegions functions obsolete. I kept the argument (and retyped it to any) to keep backwards compatibility.

@fredrikhr fredrikhr changed the title Register httpRegions as dependent to globalRegions Improve httpRegion dependency tracking Apr 5, 2022
@fredrikhr fredrikhr marked this pull request as draft April 5, 2022 14:12
@fredrikhr
Copy link
Contributor Author

Hmm, so i tested this successfully in CLI, but in VS Code this does not work for some reason. One thing though: the new property breaks the debug tree view in VS Code because of circular object graphs (regions depend on global regions which are in the same file and thus in the same tree)

@AnWeber
Copy link
Owner

AnWeber commented Apr 5, 2022

@fredrikhr debug tree view is not really important. I wanted to be able to understand the internal state of my extension even without debugging (at work). But I don't think that's a major problem either, since only the current level is ever queried, right?

There may also still be an overlap between changes from us, as #243 affects the exact same code locations. I am currently considering if and how I can possibly adjust the loopMetaDataHandler so that it integrates better into the normal flow. And doesn't need any special solutions. It has code from createRequestInterceptor, httpRegionUtils (with some code missing => #242) and now your dependency tracking :-(

if (!next.done) {
  utils.report(context, `${next.value.index} loop pass`);
  Object.assign(context.variables, next.value.variables);
  await utils.logResponse(context.httpRegion.response, context);
  context.httpRegion = this.createHttpRegionClone(context.httpRegion, next.value.index);
  if (this.request) {
    context.request = cloneDeep(this.request);
  }
  hookContext.index = this.index;
}

@fredrikhr
Copy link
Contributor Author

Ah, I found the reason why this did not work in VS Code. Previously there was a conditional in processHttpRegionActions which checked for both context.processedRegions and whether the region was global or not. My dependency tracking only cares about the latter.

But then I needed to rebase onto main and the conditional has been removed, so I now only added the check for global regions accordingly.

This now works both in CLI and VS Code.

@fredrikhr fredrikhr marked this pull request as ready for review April 6, 2022 09:21
@AnWeber
Copy link
Owner

AnWeber commented Apr 21, 2022

Sorry, I had to change the logic of loopMetaDataHandler as announced. Could you please check the conflicts and test your changes again. I think your change in the loopMetaDataHandler is no longer needed. Thanks.

@fredrikhr
Copy link
Contributor Author

@AnWeber rebased and tests showing green 👍
I have actually compiled a local vscode-extension vsix that I have been running on my machine in the last days... So far the dep-tracking seems to work great... By we should take a look at the Debug tree view in the vscode extension.

@AnWeber AnWeber merged commit 6444b34 into AnWeber:main Apr 25, 2022
@AnWeber
Copy link
Owner

AnWeber commented Apr 25, 2022

thx:-)

@fredrikhr fredrikhr deleted the ref-dep-global-region branch April 26, 2022 07:05
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