Skip to content

Nunjuck Tag Performance enhance in editor[INS-4243]#7922

Merged
cwangsmv merged 9 commits intodevelopfrom
feat/improve-nunjuck-tag-rendering
Sep 18, 2024
Merged

Nunjuck Tag Performance enhance in editor[INS-4243]#7922
cwangsmv merged 9 commits intodevelopfrom
feat/improve-nunjuck-tag-rendering

Conversation

@cwangsmv
Copy link
Contributor

@cwangsmv cwangsmv commented Sep 6, 2024

Background
Before the after-response script feature, users are using response tag to save response body as environment variable.

Due to the limitation of current Nunjuck-tag design, for every response tag used in environment variable, we will send a separate request if the response meets re-send condition during the request sending process.

For legacy users using numbers of response tag as environment variable, they will find it very slow on sending requests even they do not use that environment variable.

We will add a warning message when users are editing environments and response tag is found as a variable value which suggests users to use after-response script instead.

Changes:

  • Use cached renderContext for multiple Nunjuck tags rendering to avoid unnecessary duplicate getRenderContext call in code-editor
before.mp4
after.mp4
  • Add a warning message suggesting user to use after-script response in environment variable instead of response tag
    Screenshot 2024-09-06 at 13 35 49Screenshot 2024-09-06 at 15 13 19

Ref: [INS-4243] #4645

@notjaywu notjaywu marked this pull request as draft September 10, 2024 03:01
@cwangsmv cwangsmv marked this pull request as ready for review September 11, 2024 02:02
@jackkav
Copy link
Contributor

jackkav commented Sep 11, 2024

Cool PR, but its my understanding there are already two renderContext caches that exist in the code. Would this be adding a third?

Edit I tihnk there is one but it doesn't look well written, take a look and check it doesn't conflict or flight the cache you are introducing.
packages/insomnia/src/ui/context/nunjucks/use-nunjucks.ts

@gatzjames gatzjames force-pushed the feat/improve-nunjuck-tag-rendering branch from 4ebf032 to 9abb38d Compare September 11, 2024 10:30
render: HandleRender,
mark: CodeMirror.TextMarker<CodeMirror.MarkerRange>,
text: string,
renderContext: HandleGetRenderContext | Awaited<ReturnType<HandleGetRenderContext>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment suggesting some minor optimizations that are not blocking for this PR. Overall, the code looks good. However, the cache implementation is somewhat unclear. It would be beneficial to add documentation to explain how the cache works. This will help make future updates to the code-editor/nunjucks rendering easier to manage, given its current complexity.

@cwangsmv
Copy link
Contributor Author

Add comment for the renderContextCache implementation reason and how it works

@jackkav
Copy link
Contributor

jackkav commented Sep 12, 2024

In the comment of how it works could you also include some indications about the cache lifespan, setting and invalidation mechanism. That way it might be easier to reason about how it might interact with other parts.

@cwangsmv
Copy link
Contributor Author

cwangsmv commented Sep 12, 2024

Cool PR, but its my understanding there are already two renderContext caches that exist in the code. Would this be adding a third?

Edit I tihnk there is one but it doesn't look well written, take a look and check it doesn't conflict or flight the cache you are introducing. packages/insomnia/src/ui/context/nunjucks/use-nunjucks.ts

Great idea. Commit a new change so that the cached RenderContext will be shared between render and renderContext function in nunjuck-tags rendering.

@cwangsmv cwangsmv force-pushed the feat/improve-nunjuck-tag-rendering branch from 9754b80 to c757e99 Compare September 18, 2024 12:30
@cwangsmv cwangsmv enabled auto-merge (squash) September 18, 2024 12:30
@cwangsmv cwangsmv merged commit 684d54c into develop Sep 18, 2024
@cwangsmv cwangsmv deleted the feat/improve-nunjuck-tag-rendering branch September 18, 2024 12:58
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.

3 participants