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

Unable to override Liquid include arguments locally after upgrading to v1.0.0-beta.1 #2000

Closed
AleksandrHovhannisyan opened this issue Oct 6, 2021 · 11 comments
Labels
bug: dependency A problem in one of Eleventy’s dependencies on-hold release: beta A release on the beta channel template language: liquid

Comments

@AleksandrHovhannisyan
Copy link
Contributor

AleksandrHovhannisyan commented Oct 6, 2021

Describe the bug

On beta 1 (which appears as canary 44?), one of my includes is breaking because I'm doing this:

{%- assign arg = '12' | append: arg -%}

Somewhere else:

{% include theFile.html arg: '34' %}

Expected result: arg is now 1234 locally, within the context of the include file.

Here's the line: https://github.com/AleksandrHovhannisyan/aleksandrhovhannisyan.com/blob/master/src/_includes/img.html#L2

Unfortunately, while this works on 0.12.1, it does not work in beta 1. I logged the value of the overridden variable before and after that line, and it remains unchanged.

To Reproduce
Steps to reproduce the behavior:

  1. Install beta 1.
  2. Use Liquid.
  3. Create an include file with a single argument.
  4. Use the include and provide an argument.
  5. Attempt to reassign the argument locally within the include.
  6. Log its value before and after.
  7. Observe that it's the same value.

Expected behavior
It's weird that this changed, so I'm wondering if it's something on Liquid's end that changed to disallow this kind of overriding. If I rename the local variable so that it doesn't shadow the include's argument, then I don't get the error anymore.

Environment:

  • Eleventy: 1.0.0-canary.44 (attempted install of beta 1 is showing as this version for some reason)
  • Liquidjs: 9.28.0

Additional context

I'm using a custom Liquid config as follows:

const { Liquid } = require('liquidjs');
eleventyConfig.setLibrary("liquid", new Liquid({
  extname: '.liquid',
  dynamicPartials: false,
  root: ['src/_includes'],
}));
@zachleat
Copy link
Member

zachleat commented Oct 7, 2021

I’d try again to install Beta.1. You’re likely running into #1995, which is fixed on Beta.1 but broken on the canary releases.

@zachleat
Copy link
Member

zachleat commented Oct 7, 2021

You should be able to use the installation instructions on https://www.11ty.dev/blog/eleventy-v1-beta/

@zachleat zachleat added duplicate education release: canary A release on the canary channel and removed needs-triage labels Oct 7, 2021
@AleksandrHovhannisyan
Copy link
Contributor Author

Removed and reinstalled, and now the version is correct: 1.0.0-beta.1. However, I'm still running into the same problem 😞

Other things I've tried:

  • Removing liquidjs and instead relying on the setLiquidOptions API.

Additional context:

Now, I get this warning on startup:

[@11ty/eleventy-upgrade-help] Plugin Compatibility Error This project requires the Eleventy version to match '>= 1.0.0 < 2.0.0 || >= 1.0.0-canary < 2.0.0' but found 1.0.0-beta.1. Use `npm update @11ty/eleventy -g` to upgrade the eleventy global or `npm update @11ty/eleventy --save` to upgrade your local project version.

I guess this is why it was defaulting to canary 44 before. If I run yarn interactive-upgrade, it shows eleventy in red. Here are my dev dependencies, if it helps: https://github.com/AleksandrHovhannisyan/aleksandrhovhannisyan.com/blob/master/package.json#L34

@AleksandrHovhannisyan
Copy link
Contributor Author

AleksandrHovhannisyan commented Oct 7, 2021

@zachleat I didn't have time to throw together a minimally reproducible example yesterday, but I just created a repo for it now: https://github.com/AleksandrHovhannisyan/11ty-sandbox.

Steps to reproduce:

  1. Clone the repo, run yarn to install dependencies.
  2. Run yarn serve.
  3. Observe that 34 gets logged twice.
  4. Observe that the include file has these contents:
{{ arg | log }}
{%- assign arg = '12' | append: arg -%}
{{ arg | log }}

Expected behavior

arg gets overridden locally, and you see these logs:

34
1234

Environment

The only dependency is @11ty/eleventy at 1.0.0-beta.1.

@zachleat
Copy link
Member

zachleat commented Oct 9, 2021

I spent a little time looking at this and I think it needs to be filed upstream at https://github.com/harttle/liquidjs

Good needs is that repo is well maintained!

I would note that this exhibits the same issue (logging 34 twice):

{{ arg | log }}
{%- assign arg = 12 -%}
{{ arg | log }}

(tested in both liquidjs 9.25.1 and 9.28.0)

@zachleat zachleat added release: beta A release on the beta channel bug: dependency A problem in one of Eleventy’s dependencies template language: liquid on-hold and removed duplicate education release: canary A release on the canary channel labels Oct 9, 2021
@AleksandrHovhannisyan
Copy link
Contributor Author

Sounds good! And thanks for the simplified test case + version numbers. I'll file a bug over on their repo.

@zachleat
Copy link
Member

when this gets fixed, just as an FYI it may be blocked by #1995 because we’re locked in liquidjs 9.25 right now

@AleksandrHovhannisyan
Copy link
Contributor Author

Oh, that's unfortunate

I don't want to bug the owner of the repo, so I'm waiting to hear back on the status. I fixed this issue but broke other tests 😅

@AleksandrHovhannisyan
Copy link
Contributor Author

I believe this is resolved now thanks to #2071. Going to close, but feel free to reopen!

@AleksandrHovhannisyan
Copy link
Contributor Author

Worth noting that the fix was only for {% render %}, not {% include %}, which is deprecated. So maybe it's not fixed? 🤷

https://github.com/harttle/liquidjs/releases/tag/v9.28.4

@AleksandrHovhannisyan
Copy link
Contributor Author

AleksandrHovhannisyan commented Nov 13, 2021

Hmm, yeah, it looks like this is still a problem on beta 5; the solution is to use {% render %} instead of {% include %} according to the PR that got merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: dependency A problem in one of Eleventy’s dependencies on-hold release: beta A release on the beta channel template language: liquid
Projects
None yet
Development

No branches or pull requests

2 participants