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: merging issue with global local prop #86

Merged
merged 2 commits into from
Feb 19, 2024
Merged

Conversation

juliovedovatto
Copy link

The intent of this PR is to address an issue when using posthtml-include plugin on a page with several <include /> blocks.

Problem:

I noticed some values were being persisted across different includes, creating a larger locals object file, even though the component doesn't send any locals at all.

An example to illustrate the issue:

<!-- a.html-->
<p>Text value is: <strong>{{  props.text || "none" }}</strong></p>
<include src="./a.html">{ "props": { text: "Lorem Ipsum" } }</include>

<include src="./a.html"></include>

Generated ouput:

<p>Text value is: <strong>Lorem Ipsum</strong></p>

<p>Text value is: <strong>Lorem ipsum</strong></p>

Basically, the culprit is the Object.assign call here:

locals: posthtmlExpressionsOptions.locals ? Object.assign(posthtmlExpressionsOptions.locals, localsJson) : localsJson

The method call is basically copying localsJson inside the global locals object. And that is not the desired effect, I suppose.

The fix was simple, I basically updated this faulty line to use object propagation to merge global locals with object scoped locals into a new object.

locals: posthtmlExpressionsOptions.locals ? {...posthtmlExpressionsOptions.locals, ...localsJson} : localsJson

Generated output:

<p>Text value is: <strong>Lorem Ipsum</strong></p>

<p>Text value is: <strong>none</strong></p>

Notes:

  • I could have passed an empty object as the first parameter to Object.assign, that would have solved this problem too. Let me know if you prefer this approach instead.
  • Case you prefer, I can create a reproduction repo for this bug. But the examples provided should be enough to illustrate the situation

@Scrum
Copy link
Member

Scrum commented Feb 16, 2024

@juliovedovatto @konnng-dev resolve please

@konnng-dev
Copy link
Contributor

@Scrum This PR was sitting idle for a while, my apologies for not noticing the merge conflict.

Well, the issue was addressed. I hope the changes helps everyone passing w/ the same problem.

@Scrum Scrum merged commit 02a2a36 into posthtml:master Feb 19, 2024
3 checks passed
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