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

v2.5.0 broke the slug option #135

Assignees
Labels

Comments

@emmercm
Copy link

emmercm commented Jan 19, 2023

Describe the bug

Permalinks is ignoring a custom function given via the slug option.

To Reproduce

Given this config:

https://github.com/emmercm/www/blob/6d190e3ee94fa142de6b2377e66e5c8ba24bbdde/index.js#L436-L445

there is something going on with this code:

permalinks/src/index.js

Lines 169 to 171 in c7a1436

if (Array.isArray(options.linksets)) {
options.linksets = options.linksets.map(normalizeLinkset)
}

where the linksets end up using the default slugify function and not the provided one.

Expected behaviour

The custom slug function is called.

Screenshots

N/A

Environment

Should be OS and Node.js agnostic, but this appears on macOS and in Ubuntu CI, on Node.js 16.

Additional context

Nothing that I can think of.

@webketje webketje added the bug label Jan 19, 2023
@webketje webketje self-assigned this Jan 19, 2023
@webketje
Copy link
Member

Thx for reporting, I will fix this in a patch release ASAP.

@webketje
Copy link
Member

@emmercm would you mind testing temporarily with npm i github:metalsmith/permalinks#bugfix/#135 and if you validate I can publish?
The problem with this plugin is that it has a lot of configuration matrices and some edge cases remain untested and it does so many things that it is a pain to maintain.
Its structure is not optimal, so in 3.0 I will make a distinction between "global" options & "linksets" (e.g. I think there is little use in having per-linkset "slug" or "trailingSlash" or "directoryIndex" options . The default linkset will be added to the end of the linksets array and will define a pattern :dirname/:basename (these vars will be made available by permalinks for the pattern option).

@emmercm
Copy link
Author

emmercm commented Jan 20, 2023

@webketje I agree with all of your thoughts there. I don't mind testing at all! But I'm going to be out of town this weekend and may not get a chance until later next week.

@webketje
Copy link
Member

@emmercm any news on this?

@emmercm
Copy link
Author

emmercm commented Jan 31, 2023

I didn't realize npm was able to install from GitHub branches like that, that's awesome.

Looks like index.js wasn't committed, though: https://github.com/metalsmith/permalinks/tree/bugfix/%23135/lib

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../www/node_modules/@metalsmith/permalinks/lib/index.js' imported from .../www/index.js

@webketje
Copy link
Member

webketje commented Jan 31, 2023

Ah sorry, I forgot there's a build step indeed (which is not versioned). Here's the built version: permalinks-bugfix-351.zip
(yes you can also npm install zips, download the pkg to your site's repo root and run npm install ./permalinks-bugfix-351.zip)

@emmercm
Copy link
Author

emmercm commented Jan 31, 2023

I'm pretty sure that branch did the trick @webketje , it looks to be using my slug function again. Thank you for the fast turnaround!

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