-
-
Notifications
You must be signed in to change notification settings - Fork 519
Begin process of decoupling all templating languages #3747
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
base: main
Are you sure you want to change the base?
Conversation
48495a8
to
8af758e
Compare
8af758e
to
5d253ff
Compare
How do you tell apart async from sync Nunjucks filter (in the summary)? |
Right now some obscure Nunjucks async stuff is broken, I'm working on fixing it. This is unrelated to your question, however.
Nunjucks testcases with well formed async code already pass. The only thing broken is the ugly nunjucks callback syntax currently encouraged in the docs. I plan to workaround this by adding an internal // Example of bad code
eleventyConfig.addNunjucksAsyncFilter(
"myAsyncFilter",
function (value1, value2, callback) {
setTimeout(function () {
callback(null, "My Result");
}, 100);
}
);
// new
eleventyConfig.addFilter( "myAsyncFilter", async () => {await new Promise(() => setTimeout(...))}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to start by moving the *Dir
refactorings out into a separate PR.
They feel scoped enough that it should pass quickly without much ado.
return ret; | ||
} | ||
} | ||
else if (isAsync) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong but it doesn't look like you need an else if
because isAsync
is Boolean already.
// Note that `callback` is already a function as the `#add` method throws an error if not. | ||
let ret = filters[name].call(this, ...args); | ||
if (ret instanceof Promise) { | ||
throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this supposed to be catched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not. This is the same code addFilter added to nunjucks. It has been relocated to the nunjucks file for the purpose of decoupling
|
||
// Unfortunately this must still exist because nunjucks and liquid are not yet plugins | ||
// This will happen in my next pr | ||
__theCodeCriesInPain: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename it (and have a TODO:
in the comment - some editors have plugins that highlight those)?
Something like __slatedForRefactoring
?
@@ -1,4 +1,5 @@ | |||
function myFunction({ name }) { | |||
console.log("myFunction called",this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug survivor.
After reading the code I would have expected an
above. Actually there are three possible values: |
Yes. Just a summary and actually incorrect. The marker overrides the detection |
This pull request, codeveloped with @uncenter, dramatically simplifies the API surface of eleventy by deprecating over a dozen of language specific functions. The full scope is unknown, over time this body will be amended.
Given the huge scope, I humbly ask no darastic rewrites for one week? 🙏
The guiding vision of this pull request is to move towards a future where eleventy core does not make assumptions about templating languages, however the default experience is still just as powerful (so users opt out of additional features, not in). Why somebody might wish to do this:
The first step, which is this pull request, is to get eleventy to a place where nunjucks and other templating languages could conceivably be implemented as a plugin. Even if the broader goal is not achieved, this pull request stands great on its own by enhancing modularity and extendability.
The PR
In cases where required, the functionality of generic APIs has been extended in backwards compatible manners, and internally, all deprecated functions have been refactored to use generic & public counterparts.
We’ve attempted to ensure that as part of the refactor of deprecated functions, the documented functionality remains the same. We expect that unholy code relying on undocumented features may break. In some cases breaking changes were unavoidable. Cases where this was done as an explicit choice is listed below with the keyword "breaking"
Filters
Bad code on top of Nunjuck’s bad async code may break in different and unpredictable ways.
addNunjucksFilter
in favour ofaddFilter(..,..,{langs: ["njk"]})
addAsyncNunjucksFilter
in favour ofaddFilter(..,..,{langs: ["njk"]})
addLiquidFilter
in favour ofaddFilter(..,..,{langs: ["liquid"]})
langs
attribute toaddFilter
so that languages can filter by lang ingetFilters
Breaking: Note that one limitation is that you can’t have two different implementation of filters with the same name targeting different languages. The same holds true for shortcodes. I can think of some crafty workarounds but I struggle to think of cases where this would be desired.
Shortcodes
addNunjucksShortcode
addNunjucksAsyncShortcode
addPairedNunjucksAsyncShortcode
addPairedNunjucksShortcode
addLiquidShortcode
JavaScript
addJavaScriptShortcode
addPairedJavaScriptShortcode
addJavaScriptFilter
addJavaScriptFunction
in favour ofaddFilter
(not done yet; tbd)With JavaScript, we're outright removing a number of functions that do exactly the same thing and aren't publicly documented.
Tags
addLiquidTag
→amendLibrary("liquid",liquid => liquid.registerTag)
addNunjucksTag
→amendLibrary("njk",env => env.addExtension)
markdownTemplateEngine
&htmlTemplateEngine
thought is
markdownTemplateEngine: "njk"
becomingWhich is a combination of "Aliasing an Existing Template Language" and "Overriding or Extending an Existing Template Language." Now, Instead of having a template engine with, optionally, a html and markdown preprocessor, it holds a list of engines of infinite length. Counterintuitively, this change results in hundreds of lines worth of code reduction.
Breaking: Note that now, users overwriting
md
need to specifykey: ['liquid','md']
. The topmostTemplateRender
This will become its own PR due to a lot of complexity
Misc
addMarkdownHighlighter
→amendLibrary("md",md => md.set)
Not in this PR: configuration related methods:
setLiquidParameterParsing
,setLiquidOptions
,setNunjucksEnvironmentOptions
. While it is tempting to tell people to do library overrides, this requires "installing" (even if just to make accessible) additional packages and is thus not sufficiently beginner friendly, so these will be saved for a later pull requestGood chance all of these get deferred for later as they're more controversial unless a good simple solution emerges
Finishing touches
Linked Issues #3652, #3647. I've opened this pull request early in the process to make the intent clear
Totals: 16 deprecations