[PROPOSAL] Make all loaders asynchronous-only and Promise-based.

[PolyPik]

@RobLoach
What I am proposing will introduce breaking-changes, thus before I even make a draft PR, I wish to discuss this in Issues.

Currently, the engine provides the user the option of choosing to load templates synchronously or asynchronously. This makes sense for the file loader as Node provides synchronous and asynchronous ways to read files on the filesystem. However, for ajax the option of having it perform synchronously defeats the entire point of ajax which is suppose to be inherently asynchronous.

At first, I've tried to make the ajax loader async-only and allow for sync and async loading for the file loader. But I then encountered issues when I tried to ajax load templates that use other templates (i.e. include, extends, embed, import). The code that is suppose to handle these associations work on the assumption that the loader being used is able to perform a synchronous load.

Given what I've learned, I propose that the code be refactored in a way that assumes that all loaders are asynchronous-only. If the user wants to synchronously load a template, that can be achieved via the data key when providing params to the engine.

In addition, given that the project supports a minimum Node version of 6 (as indicated by the .travis.yml), I also propose that we deprecate the current use of callbacks to implement asynchronous behavior in favor a Promise-based implementation. Doing so would also allow for the server, in addition to the browser, to perform ajax loading through the use of Promise-based http clients such as axios.

[PolyPik]

I've already discovered this a few weeks before, but now I want to bring it to everyone's attention.

In the process of changing all the loaders to async only, I discovered that the change would also result in template rendering also becoming async only. The reason for this is because the action of rendering can involve template loading, due to the use of tags like include, extends, and embed, in the initial template.

If template loading happens asynchronously, then template rendering must happen asynchronously as well. Given that making all loaders asynchronous only would result in a breaking change to the engine's public API, should we still proceed with this course of action?

[PolyPik]

@dave-irvine, @RobLoach, @willrowe

Should we proceed?

[willrowe]

I have re-written the load a couple of times to be async-only before the hybrid implementation was done a while back, so I'm definitely on board for loading and rendering being async-only.

I wasn't involved in the hybrid implementation, so I'm not sure why it was done that way or if there is anything to take into account for that. I can look over the old issues and PRs for it though and double-check if that would be helpful.

[PolyPik]

I have re-written the load a couple of times to be async-only before the hybrid implementation was done a while back, so I'm definitely on board for loading and rendering being async-only.

I wasn't involved in the hybrid implementation, so I'm not sure why it was done that way or if there is anything to take into account for that. I can look over the old issues and PRs for it though and double-check if that would be helpful.

I think it was for backwards compatibility reasons, but you are to free look over past discussions and commits to better find out the reasoning behind the current design.

[PolyPik]

I found it. #457

[willrowe]

Thanks for finding that. It does indeed seem to be for BC sake. If everyone is on board with going the async-only route, I say we should roll this into the upcoming v2.0.0 release since that includes BC breaks as well.

[larowlan]

Hitting up against this too, trying to make a vite based loader for use with storybook, but unable to make a sync version

[willrowe]

@larowlan

trying to make a vite based loader for use with storybook, but unable to make a sync version

Do you mean "sync" or "async" there? Can you explain in more detail what is preventing you from being able to create a loader?

[larowlan]

When a template is loaded as a parent, the loader has to be able to operate in a sync fashion

see this hunk in Twig.Template.prototype.render

if (!parentTemplate) {
            url = Twig.path.parsePath(template, template.parentTemplate);
            parentTemplate = Twig.Templates.loadRemote(url, {
              method: template.getLoaderMethod(),
              base: template.base,
              async: false,
              id: url,
              options: template.options
            });
          }

The explicit async: false there means my loader can't return a promise, because immediately after, the code calls

return template.parentTemplate.renderAsync(state.context, {
            blocks: state.getBlocks(false),
            isInclude: true
          });

With the ajax loader, it can get around this because XMLHttpRequest.open has a sync flag.
Similarly the fs loader can use the sync versions of the Node fs functions.
But for vite, I have a set of glob imports that reference the raw templates. In this situation I don't have an option to make it sync (unless I'm missing something)

I've experimented with returning an empty template with an overridden .renderAsync function that can resolve the promise and then call render on the template, but that is throwing an Exception to say I'm mixing sync and async (from inside Twig.js's potentiallyAsyncSlow method)

[willrowe]

This sounds like it may be a bug. Can you open a new issue with the code you are trying to run?

[larowlan]

Can do