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

path or permalink not added to file’s metadata #136

Closed
epreston opened this issue Sep 26, 2023 · 4 comments
Closed

path or permalink not added to file’s metadata #136

epreston opened this issue Sep 26, 2023 · 4 comments

Comments

@epreston
Copy link

Upgrading from 2.4.1 to 2.5.1 the path property is not added to the file’s metadata.

I have tried path, permalink, file.path, file.permalink

Follow the steps described in the metalsmith documentation. See 3rd paragraph here:
https://metalsmith.io/docs/getting-started/#the-plugin-chain

It notes that the path property should be added.

We have been using this config. I don't know if this pattern has ever been valid. It may have been relying on invalid / empty pattern behaviour that populates the path metadata.

    .use(permalinks({
        pattern: ':filename'
    }))

We are using this metadata in a handlbars helper. Called locale-url that accepts 2 parameters.

<link href="{{locale-url 'en' path}}">

For completeness, here is is the handlebars helper that simply makes the URL fully qualified. I've added a debug helper to warn when path is not defined.

handlebars.registerHelper('locale-url', (locale, relativeUrl) => {
    if (!relativeUrl) {
        console.warn(`build.mjs : undefined relativeUrl passed to helper.`);
        return '';
    }

    relativeUrl = path.join(locale, relativeUrl.substring(2));
    const url = new URL(relativeUrl, 'https://example.com/');
    return url.href;
});

Expected behaviour
Permalinks plugin operates as documented in the 3rd paragraph here:
https://metalsmith.io/docs/getting-started/#the-plugin-chain

Either path or some replacement like permalink is defined.

Environment

  • Mac, Linux, Windows
  • Node.js 18.17.1 and 18.18.0 and 20.+

Additional context

I'd prefer to be guided toward the correct usage which will be default in the upcoming versions. Looking at the commits, the words deprecated are close to words that sound like this issue / and the functionality that we are using.

Issue on dependent project: playcanvas/developer.playcanvas.com#521

Thank you.

@webketje
Copy link
Member

Hi there,
Thank you for identifying an urgently required update to those docs (they should show permalink instead of path), I'll create an issue at https://github.com/metalsmith/metalsmith.io.

The relevant lines for this issue are 414-431. path is deprecated and as you can see on line 421. You can get warnings about these deprecations by doing:

metalsmith.env('DEBUG', '@metalsmith/permalinks:warn')

The :filename pattern can only work if a previous plugin sets that property to the file metadata. But the behavior you are looking for (namely: do not alter the destination path) can be achieved currently by omitting pattern, or setting pattern: null.

I sketched a plan for future versions in #135 (comment):

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).

So the current null value will be default to :dirname/:basename in 3.0.

@epreston
Copy link
Author

epreston commented Sep 27, 2023

Kind of you to respond so quickly. We'll switch off of using path.

Even with pattern removed or set to "null", neither path nor permalink are being added to the file's metadata. They both remain undefined.

@webketje
Copy link
Member

Ok, upon reading the linked issue I understand better. These lines of code: https://github.com/metalsmith/permalinks/blob/v2.5.1/src/index.js#L421-L431 require some rework (for one the setter should set data.permalink instead of permalink) and they are not unit-tested.

I'll include this in a patch release ASAP

@webketje
Copy link
Member

I'm closing this as it is not an issue with @metalsmith/permalinks, see playcanvas/developer.playcanvas.com#521 for more info on the resolution

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

No branches or pull requests

2 participants