-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
Adds slugify
filter as a soft-replacement for slug
#1873
Conversation
src/Filters/Slugify.js
Outdated
const slugify = require("@sindresorhus/slugify"); | ||
|
||
module.exports = function(str, options = {}) { | ||
return slugify(str, options); |
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.
Nit: Do we want to add any default options here as well? (since changing defaults is impossibly difficult to change in the future, as we're seeing).
For example, what about the decamelize
option?
https://npm.runkit.com/%40sindresorhus%2Fslugify
const slugify = require("@sindresorhus/slugify@1.1.0");
console.log(slugify('fooBar 123 $#%'));
// Output: "foo-bar-123"
console.log(slugify("Peter DeCamelize deHaan"));
// Output: "peter-de-camelize-de-haan"
console.log(slugify("Peter DeCamelize deHaan ", {decamelize: false}));
// Output: "peter-decamelize-dehaan"
console.log(slugify("random !=?\"' chars"));
// Output: "random-chars"
Note how it changes my goofy "deHaan" name to a slugified "de-haan" by default, unless we turn decamelize
off.
I like the options
for tweaking slug options. But it's also on a per-slug basis. Guessing adding a global config option eleventyConfig.setSlugOptions({ lowercase: true, decamelize: false })
gets far too awkward since we'll now have two duelling slug implementations with different options. But at least if you could set the options globally (like we can with certain engines, ie: eleventyConfig.setLiquidOptions({})
, and the slug/slugify filter somehow merges the slug filter options, with the global slugOptions
...).
But at least that way I could set eleventyConfig.setSlugOptions({decamelize: false})
globally and have it applied to each slugify
filter automatically. Versus having to put {{ author.name | slugify: options }}
. And now I'm wondering how I'd pass the options object in a filter. In the past, I'd have to define an object via frontmatter or in a global/directory data file since I don't remember if you can do {{ name | slugify: {decamelize:false} }}
Although, I guess honestly at that point, you're probably just require()
ing @sindresorhus/slugify or slugify and creating your own custom filter and overwriting the built-in "slug" filter.
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.
since I don't remember if you can do
{{ name | slugify: {decamelize:false} }}
Turns out this was pretty interesting. It worked pretty easy in Nunjucks, but quietly failed in LiquidJS (and I didn't check the other engines).
I shimmed your new slug
filter code into Eleventy v0.12.1 so it now takes a single argument, which is an object:
<!-- Nunjucks -->
<p>{{ "peter 'Nunjucks' DeHaan" | slug({replacement:"!"}) | safe }}</p>
<!-- Output: "<p>peter!'nunjucks'!dehaan</p>" -->
So looks like Nunjucks supports passing object as filter parameters. Neato.
<!-- LiquidJS -->
<p>{{ "peter 'Liquid' DeHaan1" | slug: {replacement:"!"} }}</p>
<!-- Output: "<p>peter-'liquid'-dehaan1</p>" -->
But it looks like LiquidJS didn't use the custom replacement
value.
If I dump the incoming str
and options
code in my ./node_modules/@11ty/eleventy/src/Filters/Slug.js file, it seems like I'm getting this:
DEBUGGERY: { str: "peter 'Liquid' DeHaan1", options: {} }
So my custom options were dropped. 🤷
Back to my frontmatter object trick:
---
slugOpts:
replacement: "?"
---
<p>{{ "peter 'Liquid' DeHaan1" | slug: {replacement:"!"} }}</p>
<p>{{ "peter 'Liquid' DeHaan2" | slug: slugOpts }}</p>
<!-- Output: "<p>peter?'liquid'?dehaan2</p>" -->
That worked, so I think that means that LiquidJS doesn't support passing objects as filter parameters and something is getting stringified or undefined in the pipeline. I'll have to see if I can create a simple liquidjs test case and file it upstream in their repo and see what the LiquidJS team says.
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.
LiquidJS filter object arguments issue filed upstream as harttle/liquidjs#365.
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 did switch the decamelize option off by default and added tests for the options passing. I didn’t add global defaults switching yet, I’m kinda with you on the wholesale filter replacement method rather than adding new config API for it.
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.
Open PR related to Liquid parsing: #1733 We do have a layer of custom parsing already in place here in Eleventy for shortcodes specifically but we can look at this in a separate issue.
…to decamelize default for slugify filter.
Really appreciate all of the feedback @pdehaan—I made some changes and added tests! Mergin’ |
First step to fix #278. This PR is open to review and welcomes feedback.
Remaining steps:
slug
slugify