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

Slug filter doesn't create url safe slugs. #278

Closed
edwardhorsford opened this issue Oct 17, 2018 · 53 comments · Fixed by #1873
Closed

Slug filter doesn't create url safe slugs. #278

edwardhorsford opened this issue Oct 17, 2018 · 53 comments · Fixed by #1873

Comments

@edwardhorsford
Copy link
Contributor

I've been using the slug filter to convert regular strings to strings suitable for use as urls. However I've realised it's not set up to remove apostrophes (and possibly other characters?).

So it's a test becomes it's-a-test, and when rendered with eleventy the url is /tags/it's-a-test/ - the ampersand breaks the url. It also looks ugly!

I see that the package supports supplying a list of characters to remove.

If the intention is that this filter is used to create 'safe' urls, could Eleventy remove these by default?


As an alternative I've added my own filter using the slugify() option of the string library. I might stick with this anyway as it seems to do a good job of creating 'pretty' slugs.

@hughrun
Copy link

hughrun commented Oct 21, 2018

I just noticed a similar problem with commas in the title - it leaves the commas in for the automatically slugified permalink.

@zachleat
Copy link
Member

Oh, hmm. Well that’s not ideal.

I did try permalink: '/{{ "Hi I''m Zach" | slug }}/' (note the escaped single quote using '') which resulted in the URL http://localhost:8080/hi-i'm-zach/ which worked in both Chrome and Firefox. Where is the conversion happening in your URL? It doesn’t seem to be coming from the slugify filter.

@zachleat zachleat added the open-question Requires additional information before we can proceed. label Nov 18, 2018
@edwardhorsford
Copy link
Contributor Author

I think the point is more that I'd expect apostrophes to be be stripped by the filter. And that it would generally make 'nice' urls.

In my case this was most obvious when using automatic anchor links from headings - or from tags from tag pages.

@zachleat
Copy link
Member

Well as much as I agree with you, this would be a breaking change and would require a major version bump. So I'm trying to figure out the exact impact of this to determine priority. It doesn't seem to be breaking anything in the test case I tried.

@edwardhorsford
Copy link
Contributor Author

Oh totally! Quite a bad breaking change too. Even with a major version bump I suspect you'll want / need to offer a compatibility mode. Although I it only impacts a certain percentage of links, and users will likely have avoided using them, it has the potential to screw up lots of links.

Some thoughts:

  • You could expose more of the config of the base package so users can strip these things if they want.
  • You could offer a higher level config that users opt in to (and later make default, and offer a 'legacy' option)

FWIW, I think I prefer the string library's implementation anyway - to me it handles more edge cases nicer. I'll stick with my own filter anyway.

@zachleat zachleat added this to the Next Major Version milestone Nov 22, 2018
@zachleat zachleat added needs-discussion Please leave your opinion! This request is open for feedback from devs. and removed open-question Requires additional information before we can proceed. labels Nov 22, 2018
@jevets
Copy link

jevets commented Nov 30, 2018

@edwardhorsford

Out of curiosity:

  • Are you replacing the slug filter with your own implementation? (Does Eleventy allow replacing the built-in filter?) And still using the same filter name: {{ title | slug }}?

  • Or are you using your own complete implementation, something like {{ title | edsSpecialSlugifyFilter }}?

I think it makes sense to expose config/options on the built in filter (like @edwardhorsford says above), opt-in only; keep the defaults as-is and avoid breaking changes.

@edwardhorsford
Copy link
Contributor Author

edwardhorsford commented Nov 30, 2018

@jevets the latter - a new filter with a new name. FWIW I went with slugify

I'm not sure the long term aim should be to keep as-is - though changing it presents issues and challenges. Fundamentally (to me at least), this filter is for making slugs / urls. It takes arbitrary string input and outputs something that can be used safely as a slug / url. I think you could argue that it's a bug that it doesn't currently do that - some arbitrary strings create slugs and urls that cannot be used.

Edit - here's the filter I'm now using:

const string = require('string')

module.exports = function(input) {
	if (!input) return false;
	else return string(input).slugify().toString();
}

@kazzkiq
Copy link

kazzkiq commented Jan 2, 2019

@edwardhorsford Did you managed to also generate the folders with this new slug function? If so, care to explain how?

I'm also having this issue where punctuation keeps adding noise to the URL, which looks odd and is awful for usability (imagine an user having to nitpick punctuations from the URL instead of just typing alphanumeric and dashes).

This:

mywebsite.com/posts/typescript:should-i-be-using-it,or-not

Versus this:

mywebsite.com/posts/typescript-should-i-be-using-it-or-not

What is easier to identify/read/type? Yeah... Medium seems to have an answer.

@edwardhorsford
Copy link
Contributor Author

@kazzkiq I think you should be able to use custom filters in permalink generation - have you tried?

@kazzkiq
Copy link

kazzkiq commented Jan 3, 2019

Yep, it worked!

So just in case anyone else ends up here, here is how to fix your slug function:

  1. Implement @edwardhorsford function slugify: (Doesn't know this file? Example here)
// .eleventy.js
eleventyConfig.addFilter('slugify', input => {
  if (!input) { return false };
  return string(input).slugify().toString();
});
  1. Find/Replace every | slug in your project, to: | slugify.
  2. Recompile your project (the new URLs may break older shared links, etc).

@zachleat
Copy link
Member

zachleat commented Jan 3, 2019

Thanks for sharing @kazzkiq! I wonder if you could just use slug? Does it overwrite the built in filter? I feel like that should work if it doesn't

@okitavera
Copy link

yup @zachleat , it does replace the slug, so I just do remove some chars to make it little bit prettier tho

const slugify = require("slugify");
eleventyConfig.addFilter("slug", (input) => {
  const options = {
    replacement: "-",
    remove: /[&,+()$~%.'":*?<>{}]/g,
    lower: true
  };
  return slugify(input, options);
});

@zachleat
Copy link
Member

zachleat commented Jan 8, 2019

Excellent, thank you @okitavera.

@zachleat zachleat added enhancement and removed needs-discussion Please leave your opinion! This request is open for feedback from devs. labels Jan 8, 2019
@zachleat
Copy link
Member

zachleat commented Jan 8, 2019

I’m going to move this into the new feature queue and it is logged for the next major version milestone.

@zachleat zachleat added the needs-votes A feature request on the backlog that needs upvotes or downvotes. Remove this label when resolved. label Jan 8, 2019
@zachleat
Copy link
Member

zachleat commented Jan 8, 2019

This repository is now using lodash style issue management for enhancements. This means enhancement issues will now be closed instead of leaving them open.

View the enhancement backlog here. Don’t forget to upvote the top comment with 👍!

@zachleat zachleat closed this as completed Jan 8, 2019
@bridgestew
Copy link

@okitavera Thanks for posting that solution. It works great for the post titles.

@zachleat or anyone: Is there a suggestion for how to make @okitavera 's solution work with markdown-it-anchor, as well? I'm not sure where to begin because I am a Javascript noob. Thanks!

@zachleat
Copy link
Member

@bridgestew are you using eleventy-base-blog?

If so your opts defined here (docs: https://github.com/valeriangalliat/markdown-it-anchor):
https://github.com/11ty/eleventy-base-blog/blob/master/.eleventy.js#L43

  let opts = {
    permalink: true,
    permalinkClass: "direct-link",
    permalinkSymbol: "#"
};

would be something like:

const slugify = require("slugify");
let opts = {
    permalink: true,
    permalinkClass: "direct-link",
    permalinkSymbol: "#",

    // this is the same function shared above
    slugify: function(input) {
      const options = {
        replacement: "-",
        remove: /[&,+()$~%.'":*?<>{}]/g,
        lower: true
      };
      return slugify(input, options);
    }
};

Does that make sense?

@bridgestew
Copy link

@zachleat It does and it worked. Thank you so much for the help!

singaround pushed a commit to singaround/songbook that referenced this issue Mar 16, 2019
If a 'tag' has an apostrophe in the name it breaks Netlify's deploy
script, which doesn't like special characters. The built-in 'slug'
function in Eleventy doesn't fix apostrophes either

11ty/eleventy#278

Using the soltion there, I've updated 'slug' to be more inclusive
and the new tag names are better
@rolandtoth
Copy link

This is only partly related, but I got an error if the frontmatter title contains : and the permalink is set like this:

"title": "Tip: Eleventy rocks",
"permalink": "/{{ title | slug }}/"

The error:

Problem writing eleventy templates (more info in DEBUG output): 
{ Error: dist/tip:-eleventy-rocks contains invalid WIN32 path characters.
    at Object.mkdirs (C:\Users\tpr\AppData\Roaming\npm\node_modules\@11ty\eleventy\node_modules\fs-extra\lib\mkdirs\mkdirs.js:18:22)
    at Object.mkdirs (C:\Users\tpr\AppData\Roaming\npm\node_modules\@11ty\eleventy\node_modules\universalify\index.js:5:67)
    at pathExists (C:\Users\tpr\AppData\Roaming\npm\node_modules\@11ty\eleventy\node_modules\fs-extra\lib\output\index.js:20:11)
    at fn.apply.then.r (C:\Users\tpr\AppData\Roaming\npm\node_modules\@11ty\eleventy\node_modules\universalify\index.js:23:46)
    at <anonymous> code: 'EINVAL' }
  • Win 8.1
  • Eleventy 0.7.1

@Ryuno-Ki
Copy link
Contributor

Ryuno-Ki commented Apr 2, 2019

Hm, experience told me, that slugging is an area where it is better to rely on a library instead of rolling an own solution.
So I looked into what's available on npm and came up with a list:

Maybe it makes sense to talk about the constrains here?
Like, which languages should be supported? What about numbers? Unicode (among them, emojis) etc.

@pdehaan
Copy link
Contributor

pdehaan commented Jun 11, 2021

For anybody else curious, here's the difference between the default slug filter (which uses slugify; currently slugify@1.5.3 in Eleventy 0.12) vs @sindresorhus/slugify@1:

default slug filter (via slugify@1.5.3) custom slugify filter (via @sindresorhus/slugify@1.1.2)
{{ Ä-ä | slug }} = "a-a" {{ Ä-ä | slugify }} = "ae-ae"
{{ Ö-ö | slug }} = "o-o" {{ Ö-ö | slugify }} = "oe-oe"
{{ Ü-ü | slug }} = "u-u" {{ Ü-ü | slugify }} = "ue-ue"
{{ ẞ-ß | slug }} = "ss-ss" {{ ẞ-ß | slugify }} = "ss-ss"
const slugify = require("@sindresorhus/slugify");

module.exports = (eleventyConfig) => {
  eleventyConfig.addFilter("slugify", slugify);
  return {...};
};

Or, if you want to replace the built-in Eleventy "slug" filter with a different implementation, just name your custom slugify filter "slug":

const slugify = require("@sindresorhus/slugify");
eleventyConfig.addFilter("slug", slugify);

NOTE: Only @sindresorhus/slugify v1 is supported (npm i @sindresorhus/slugify@1). v2 uses ESM modules and you'll get errors when trying to use it:

Error was thrown:
   Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /private/tmp/11ty-liquid-or/node_modules/@sindresorhus/slugify/index.js
   require() of ES modules is not supported.


UPDATE: For my own future reference, I managed to get async filters working in Nunjucks using addNunjucksAsyncFilter() (async filter support wasn't added into LiquidJS until v9.1.3, per harttle/liquidjs#232) and the v2/ESM version of @sindresorhus/slugify:

  eleventyConfig.addNunjucksAsyncFilter("slugify", async function(str, callback) {
    // Where @sindresorhus/slugify is v2.1.0, which is an ESM module.
    const slugify = await import("@sindresorhus/slugify");
    callback(null, slugify.default(str.toString()));
  });

If you're using Eleventy v1/Canary (which currently has liquidjs@9.25.0), this code works for an async LiquidJS filter or an .11ty.js template, but will return a Promise object in Nunjucks:

  eleventyConfig.addFilter("slugify", async (str) => {
    const slugify = await import("@sindresorhus/slugify");
    return slugify.default(str.toString());
  });
// test.11ty.js
module.exports = {
  async render(data) {
    return `slug=${ await this.slugify("Peter deHaan 'tis an idiot (11ty.js)") }`;
  }
};
STRING LANGUAGE RESULT
slug={{ "Peter deHaan 'tis an idiot (LiquidJS)" | slugify }} LiquidJS slug=peter-de-haan-tis-an-idiot-liquid-js
slug=${ await this.slugify("Peter deHaan 'tis an idiot (11ty.js)") } .11ty.js slug=peter-de-haan-tis-an-idiot-11ty-js
slug={{ "Peter deHaan 'tis an idiot (Nunjucks)" | slugify }} Nunjucks slug=[object Promise]

@Brixy
Copy link

Brixy commented Jun 12, 2021

Thank you very much. @sindresorhus/slugify@1 works perfectly with umlauts.

NOTE: Only @sindresorhus/slugify v1 is supported (npm i @sindresorhus/slugify@1). v2 uses ESM modules and you'll get errors when trying to use it

Is this a reason not to use @sindresorhus/slugify as the default for eleventy 1.0 as some suggested? Or will eleventy support ESM?

@pdehaan
Copy link
Contributor

pdehaan commented Jun 12, 2021

Is this a reason not to use @sindresorhus/slugify as the default for eleventy 1.0 as some suggested? Or will eleventy support ESM?

Oh, I wasn't suggesting 11ty change the default slugify library (although personally I think @sindresorhus/slugify is the better option). I think it'd have too many issues w/ backwards compatibility and broken/changed links. But it's an easy fix if you want to opt-in for individual sites.

@zachleat
Copy link
Member

Awesome commentary y’all—I think the safest option (even in 1.0) will be to keep the existing slug filter as-is and supplement with a new filter with a different name. slugify seems like a good name but I’m open to suggestions. Reading through the comments it looks like the @sindresorhus/slugify package might be our best bet.

This would also include a docs change to recommend the new filter as the default moving forward and deprecate the old one. That way we won’t break any sites or have to offer a compatibility mode or cumbersome link checker thing.

@pdehaan
Copy link
Contributor

pdehaan commented Jun 30, 2021

I don't know.
On one hand, switching to a different slug implementation/config will likely break a percentage of existing projects (not a huge number, only people w/ international slugs or emojis or funky characters).
On the other hand, having a built in slug and slugify filter sounds confusing.

It isn't prohibitively difficult to use a different slug implementation and overwrite the built-in slug function.

const slugify = require("@sindresorhus/slugify"); // v1
eleventyConfig.addFilter("slug", slugify);

Curious if it'd be better to wrap the old or new slugify implementation in a custom 11ty-plugin-slug Plugin and people can opt in to the old or new behavior. What if Eleventy v1 switches to @sindresorhus/slugify@1 (non-ESM-promise version), and then people that complain that their URLs break, can opt into the brand new "@11ty/eleventy-slug-legacy" plugin.

Unless we ship w/ both slugify and @sindresorhus/slugify dependencies (which I think is what you were proposing above anyways), and let people opt into one or the other using some new Eleventy config option. Maybe by default the old slugify module is used, but if you opt-in to the @sindresorhus/slugify version, you get the better i18n support and everything else. This way both libraries can use the "slug" filter name and you just specify if you want the new or old library.

@nhoizey
Copy link
Contributor

nhoizey commented Jul 1, 2021

I would go for the breaking change in 1.0 because 2.0 might take time… 😅

I've already been using @sindresorhus/slugify for a long time in all my Eleventy projects, so it's not something I need for myself, but I think it would be better for everyone new to Eleventy.

And I agree with @pdehaan that there could be a plugin for legacy behavior.

I don't know if it's possible to use the ESM version of @sindresorhus/slugify (yet?), I didn't manage to make it work in my projects.

@pdehaan
Copy link
Contributor

pdehaan commented Jul 1, 2021

I don't know if it's possible to use the ESM version of @sindresorhus/slugify (yet?), I didn't manage to make it work in my projects.

This is as close as I've ever gotten to using the ESM version in commonjs/Node. It seems to work for Liquid (9.1.3+ via eleventy@canary build) and Nunjucks (via .addNunjucksAsyncFilter()).

module.exports = function (eleventyConfig) {
  const slugifyFn = async (str) => {
    // Import the ESM module...
    const fn = await import("@sindresorhus/slugify");
    return fn.default(str);
  };

  eleventyConfig.addFilter("slugify", (str) => slugifyFn(str));
  // Note that the async Nunjucks filter uses the same name as the global `slugify` filter.
  eleventyConfig.addNunjucksAsyncFilter("slugify", (str, callback) => {
    slugifyFn(str)
      .then((value) => callback(null, value))
      .catch((err) => callback(err));
  });

Definitely easier to stick with @sindresorhus/slugify v1 (pre-ESM). I tried, but could never figure out how to get Node's util.callbackify() function to work w/ @sindresorhus/slugify, which might have been able to shave a couple lines off of that code.

@edwardhorsford
Copy link
Contributor Author

I've long since stopped using the Eleventy solution and gone with my own - so this doesn't directly impact me any more.

Personally I think this should be a breaking change with an option to revert to the old behaviour.

If there is a new filter, though I personally like slugify, I might try to find something more plain English - format-for-urls or something?

@zachleat
Copy link
Member

zachleat commented Jul 1, 2021

I appreciate the pushback!

To be clear here the slug filter using the slugify package is only a user-land filter and is not used in any way internally in core—I feel like some of the alternatives being proposed here have assumed that it was used internally.

Exposing a new name and a soft-default-change (docs update) to point to this new name has a bunch of benefits:

  1. Existing URLs using slug do not break. This is so very important. Breaking compatibility between versions is one thing. Breaking URL output consistency is a next level of bad problem we want to avoid AT ALMOST ANY COST (sorry for all caps but I really don’t want to break any URLs). Changing the default slug to point to a new package would be a not just be a breaking change but would also (in my opinion, at bare minimum) require some extra code to compare old with new URLs to warn about URL changes.
  2. Allows incremental swapping to the new filter. Projects can use the old and the new method at the same time.
  3. No configuration switches needed. A configuration option would likely be global (citation needed?).

@zachleat
Copy link
Member

zachleat commented Jul 1, 2021

That said, we could also do a tiny opt-in @11ty/eleventy-slug-upgrade plugin to update the existing slug to whatever the new name is globally. (similar to what @pdehaan suggested but flipped around to be opt-in instead of opt-out)

@zachleat
Copy link
Member

zachleat commented Jul 1, 2021

#1873 is open for feedback! I stuck with the 1.x @sindresorhus/slugify package for now and decided to stick with the slugify name for brevity and to match the upstream package name. We can revisit 2.x when more ESM stuff lands in Eleventy.

@pdehaan
Copy link
Contributor

pdehaan commented Jul 2, 2021

Purely for comedic purposes, but I think this might possibly work if we have a future [Eleventy v1] slug filter (which uses slugify), and a slugify filter (which uses @sindresorhus/slugify), but you want to alias one to the other (because you like the "slug" filter name, but newer "slugify" implementation…

eleventyConfig.addFilter("slug", eleventyConfig.getFilter("slugify"));

Yeah, it has great potential to break your existing URLs because it's a completely different implementation, but #YOLO. 💥

@pdehaan
Copy link
Contributor

pdehaan commented Jul 3, 2021

Another silly experiment… I hijacked the slug filter so it will apply both the original, built-in slug filter and the proposed slugify filter for a given string and exit with a non-zero code if they don't match (but still returns the value from the slug filter, so it shouldn't break anything… maybe).

const assert = require("assert");
const inspect = require("util").inspect;

const slugify = require("@sindresorhus/slugify");

module.exports= (eleventyConfig) => {
  const slugFn = eleventyConfig.getFilter("slug");

  const slugErrors = new Set();

  eleventyConfig.addFilter("slugify", slugify);
  eleventyConfig.addFilter("slug", function (str="") {
    const slugifyFn = eleventyConfig.getFilter("slugify");
    const slugValue = slugFn(str);
    try {
      assert.strictEqual(slugValue, slugifyFn(str));
    } catch (err) {
      // Only display a unique error once.
      if (!slugErrors.has(err.message)) {
        console.error(`\nslug-vs-slugify filter mismatch for ${inspect(str)}\n${err.message}`);
        slugErrors.add(err.message);
        process.exitCode = 2;
      }
    } finally {
      return slugValue;
    }
  });

  return {
    dir: {
      input: "src",
      output: "www"
    }
  };
};

INPUT

---
title: Peter deHaan
permalink: "{{ title | slug }}/"
---

title={{ title }}
permalink={{ page.url }}
---
slug={{ title | slug }}
slugify={{ title | slugify }}

OUTPUT (HTML)

title=Peter deHaan
permalink=/peter-dehaan/
---
slug=peter-dehaan
slugify=peter-de-haan

OUTPUT (Console)

slug-vs-slugify filter mismatch for 'Peter deHaan'
Expected values to be strictly equal:
+ actual - expected

+ 'peter-dehaan'
- 'peter-de-haan'
           ^

So, if your site still builds successfully and doesn't exit with a non-zero code, I think it's probably safe to switch from using |slug to |slugify and your links won't have changed.
If your site does exit with a non-zero code, you'd probably have to stick w/ using |slug filter, or tweak your permalink frontmatter, or whatever.

TLDR: I was bored and wanted to try building a slugify linter.


UPDATE: If you want to migrate from slug to slugify because of reasons, you can make the built-in, legacy slug filter throw a build-breaking error with the following gem:

  eleventyConfig.addFilter("slug", () => {
    throw new Error("`slug` filter is no longer supported. Please use `slugify`.");
  });

Probably limited usefulness, but if you've already migrated to slugify and don't want to accidentally use slug out of habit, it might be helpful.

Or, you could just use the fancy alias technique from above so that both slug and slugify both use the slugify filter.

@zachleat
Copy link
Member

zachleat commented Jul 7, 2021

Awesome @pdehaan—I will link to your linter from the docs at #1883

@zachleat
Copy link
Member

zachleat commented Jul 7, 2021

This PR is merged and slugify will ship with 1.0, thanks y’all!

@zachleat
Copy link
Member

zachleat commented Jul 7, 2021

@pdehaan I slightly modified your linter in a gist to link up from the docs: https://gist.github.com/zachleat/a58bc9e7273fc182a3c9c1234fee82c8

@zachleat
Copy link
Member

zachleat commented Jul 9, 2021

Just a small update here, @pdehaan your little script is going to live on in the 1.0 upgrade helper plugin:

https://github.com/11ty/eleventy-upgrade-help/

Feel free to contribute over there!

@zachleat
Copy link
Member

Moving the case sensitivity (and/or normalization) of tags to #2462, if y’all want to follow along!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.