frctl / twig

Use Twig templates with Fractal.
32 stars 34 forks source link

Nested components using the path filter fail #20

Closed Chapabu closed 5 years ago

Chapabu commented 5 years ago

I'm not sure if this happens all the time, but if you have nested components with paths passed to the | path filter, then it triggers the following error:

Error: TypeError: Cannot read property 'request' of undefined
    at /PATH/TO/FRACTAL/PROJECT/node_modules/@frctl/twig/src/adapter.js:158:24
    at new Promise ()
    at TwigAdapter.render (/PATH/TO/FRACTAL/PROJECT/node_modules/@frctl/twig/src/adapter.js:144:16)
    at ComponentSource._renderVariant (/PATH/TO/FRACTAL/PROJECT/node_modules/@frctl/fractal/src/api/components/source.js:207:30)
    at _renderVariant.next ()
    at onFulfilled (/PATH/TO/FRACTAL/PROJECT/node_modules/co/index.js:65:19)
    at tryCatcher (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:512:31)
    at Promise._settlePromise (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromise0 (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:614:10)
    at Promise._settlePromises (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:694:18)
    at Promise._fulfill (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:638:18)
    at Promise._resolveCallback (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:432:57)
    at Promise._settlePromiseFromHandler (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:524:17)
    at Promise._settlePromise (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromise0 (/PATH/TO/FRACTAL/PROJECT/node_modules/bluebird/js/release/promise.js:614:10)

Browsing through, the error looks to be coming from https://github.com/frctl/twig/blob/master/src/filters/path.js#L10-L11. We check for validity of the env object, but we're checking too late as we're using it on the line directly prior to the safety check.

Hopefully this is as quick as adding an extra check in place.

mihkeleidast commented 5 years ago

are you by any chance using the only keyword on your include tag? that's the only case when i've had this error.

unfortunately, using only strips necessary information out of context (like request) that is necessary for returning the correct path from the path filter. without it, even if we add another check, it will return a wrong path.

Chapabu commented 5 years ago

No, we're not using only at all, but it does seem like _env is being stripped. I had a quick look, but it looks like there's some magic going on. Need to take a proper look with an actual debugger rather than just console.log() 😆

julkue commented 5 years ago

Will be fixed in #22

mihkeleidast commented 5 years ago

Commented on #22 as well, but here just in case: we've been using this adapter for the last year for a lot of projects and never ran into this issue after discovering https://github.com/twigjs/twig.js/issues/287 and restricting embed use. Could it be the same issue?

I think when it comes to the path filter, the request variable should always be defined or the path filter will return an incorrect result for the static build. We could add that check around it in #22, but that will escape the error, not solve it, IMO.

Could either of you create a more specific demo that demonstrates the issue?

hstandaert commented 5 years ago

I'm encountering this issue as well in the following working example:

image/image.twig

<img src="{{ '/path/to/my/image.jpg' | path }}" alt="foo" />

teaser/teaser.twig

<div class="teaser">
    {% render "@image" %}
    {# ... #}
</div>
julkue commented 5 years ago

@hstandaert Is it solved for you when using the branch of PR #22?

hstandaert commented 5 years ago

@julmot, I'm afraid not. It does prevent the error from being thrown but doesn't return the right path at that point.

When I serve the static files generated by fractal (and located in a ./build folder for my project), the returned path is still /path/to/my/image.jpg, while /build/path/to/my/image.jpg is expected.

hstandaert commented 5 years ago

Replacing the render tag with the default {% include %} tag solved the issue for me. I did some debugging and suppose it has something to do with the wrong context being passed to the filter when a subcomponent is being rendered 🤷‍♀️

In other words:

image/image.twig

<img src="{{ '/path/to/my/image.jpg' | path }}" alt="foo" />

teaser/teaser.twig

<div class="teaser">
    {% include "@image" %}
    {# ... #}
</div>

... solved it for me

julkue commented 5 years ago

Yes, but this will not use the default context of @image

julkue commented 5 years ago

I noticed another strange behavior, maybe it's related to this issue: If you have e.g. a property header.title and a property product.title defined in your configuration yml file, and you're outputting both, they will be the same. So, one of them will have the wrong value.

maximedaoust commented 5 years ago

I'm facing the same problem. PR #28 will be a nice workaround for development but this wont work for fractal build