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

Remove legacy github slugger #872

Merged
merged 1 commit into from
Dec 21, 2021
Merged

Conversation

memeplex
Copy link
Contributor

@memeplex memeplex commented Dec 12, 2021

@riccardoferretti I'm not sure how do you regenerate yarn.lock so I've only changed package.json.

I decided to leave the slug term as it's used in many places, I think it has a legitimate meaning. Regarding the references used to find resources in the workspace, one could say a reference is:

  • An URI
  • Or a slug (string) which can be:
    • Absolute
    • Relative
    • An identifier

AFAICS the current code is consistent with this terminology.

I've added a refactor label since I expect to send a few more purely-refactoring PRs, but feel free to delete or rename it.

@riccardoferretti
Copy link
Collaborator

to regenerate yarn.lock just run yarn (no params)

@riccardoferretti
Copy link
Collaborator

Let's merge once you have updated yarn.lock

@riccardoferretti
Copy link
Collaborator

I am actually now thinking that in the other PR we make a "legitimate" use of the slugger, so although it's no longer used in the heading generation (which tbh I think is no longer used anywhere), we should keep the slugger.

At the same time, probably we don't need the full package, but just the actual slugging function from https://github.com/Flet/github-slugger/blob/master/index.js

wdyt?

@memeplex
Copy link
Contributor Author

memeplex commented Dec 16, 2021

Yes, I know. That's why I commented there. I'm not sure how legitimate it is, because:

  • To implement it properly, it should work in both directions: if you ctrl-click a placeholder [[the-slug]] it should produce a title # The Slug. That PR only proposes to have the slug available in a template. So it will result in usage inconsistencies.

  • There are many ways to slugify a title. For example, someone else will eventually request [[the.slug]] because she's used to dendron.

The direction Foam seems to be taking in this regard is title = slug = filename. I believe it's the right direction and it's the same than Obsidian does. I'm not necessarily opposed to a more general slugifying solution (two-way and configurable for different styles) but short of that I believe it's more coherent to promote the title = slug = filename convention and leave other options to be fully manual. In particular, I dislike having the github slugger dependency just to implement an ad hoc solution.

@memeplex
Copy link
Contributor Author

memeplex commented Dec 16, 2021

If we're going to keep some form of slug, the minimal implementation that seems consistent to me is:

  1. Add a configuration slugify toggle to enable slugs.
  2. Implement a pair of mutually inverse functions, say slugify and unslugify (this would be a simple implementation without external dependencies, also notice that "inverse" is an approximation because case information may be lost when slugifying).
  3. Use these functions both for templates and when creating notes for placeholders.

Alternatively the configuration may allow to customize slugs a bit, perhaps a separator character and lowercase flag. In this case there is no need to add an additional slugify toggle.

But, as I said, I'm ok with removing all traces of slugs instead. I've been checking what Obsidian does again and it doesn't even add a # Title in new notes.

@riccardoferretti
Copy link
Collaborator

The point of slugs potentially opening doors we can keep closed now is a fair one.
If we say "no slugs" there is no support for it and we are done.
If we say "little slugs" I can see more people wanting more features in the future.

But I am thinking it might be ok to provide basic support for slugs, defined as:

  • one way function to slugify a title
  • one slug format supported (basically to help you have a url friendly file name)

To implement it properly, it should work in both directions: if you ctrl-click a placeholder [[the-slug]] it should produce a title # The Slug.

If you click on [[the-slug]] you will get the-slug.md, we don't provide additional support for slugging. The title of the note will be "the-slug", the same way when you click on [[a note]] the title is "a note", not "A Note". We don't try to be smart in that situation.

There are many ways to slugify a title.

By slug I mean the github-slug, no other slugs are supported, I am comfortable drawing that line in the sand - because for me the goal in the end is the one stated above: give an easy path to URL friendly name.

I can see a case where someone wants to put a title, but wants the file name (and connections to it) to use the slug... it wouldn't me my way of doing things, but why not.
I can also see how in the future, via definitions and janitor, we might be able to support replacing a [[link-to-note]] with the corresponding [[Title of note]] plus a definition of [[Title of note]] => link-to-note.md, but I consider that to be very low priority.

The only support for slug I am happy to provide is FOAM_TITLE_SLUG, the cost/maintenance is close to zero, the only thing I am concerned about is stuff like non-latin chars, emojis, ... but then again, this is just a variable available in a template, no core logic depends on its presence, use at your own peril...

The direction Foam seems to be taking in this regard is title = slug = filename

The only thing Foam wants is that wikilink/identifier is based on filename/path, we don't have requirements around titles, slugs, or else.

I believe it's the right direction and it's the same than Obsidian does.
I've been checking what Obsidian does again and it doesn't even add a # Title in new notes.

I do like how in Obsidian the file name is the note title, but that's not possible in Foam because in Obsidian it's the editor that is "constrained" in the UI, and VS Code doesn't allow us that (and I am not sure whether I'd take that if I could). And of course you can add a title at the top, in that case you'd have filename != title.
In general it just doesn't seem like a strong requirement, but more a good convention.

I dislike having the github slugger dependency just to implement an ad hoc solution.

I totally agree, we can just copy/paste the function (in the end it's just a regex + lowercase for the simple latin case). We should remove the dependency on github-slugger

Now, all of that being said, I am actually a bit torn because of the very first 3 lines of this comment.

@memeplex
Copy link
Contributor Author

The direction Foam seems to be taking in this regard is title = slug = filename

The only thing Foam wants is that wikilink/identifier is based on filename/path, we don't have requirements around titles, slugs, or else.

Yes, I understand this and I like it to be so, I'm just saying that foam is making this convention work OOTB while still allowing other conventions (with manual intervention).

If you click on [[the-slug]] you will get the-slug.md, we don't provide additional support for slugging.

But with this content by default:

# the-slug
...

If we're going this way I'd like to do this slightly better. If you don't want the config option, perhaps the unslugify function could be implemented in a way that heuristically detect slugs, so that unslugify("the-slug") -> "The slug" and unslugify("The slug") -> "The slug". A sensible rule would be "if it's all lowercase and has no spaces, then make the first letter uppercase and replace - with space". This could be named titlefy instead of unslugify.

@memeplex
Copy link
Contributor Author

I could implement the zero-config proposal or the more elaborate proposal in #872 (comment).

@riccardoferretti
Copy link
Collaborator

The direction Foam seems to be taking in this regard is title = slug = filename

The only thing Foam wants is that wikilink/identifier is based on filename/path, we don't have requirements around titles, slugs, or else.

Yes, I understand this and I like it to be so, I'm just saying that foam is making this convention work OOTB while still allowing other conventions (with manual intervention).

If you click on [[the-slug]] you will get the-slug.md, we don't provide additional support for slugging.

But with this content by default:

# the-slug
...

That's correct, that's what I mean by "not providing first class support for slugs" :)
We also have the title selected so it's easy to change if one wishes so.

If we're going this way I'd like to do this slightly better. If you don't want the config option, perhaps the unslugify function could be implemented in a way that heuristically detect slugs, so that unslugify("the-slug") -> "The slug" and unslugify("The slug") -> "The slug". A sensible rule would be "if it's all lowercase and has no spaces, then make the first letter uppercase and replace - with space". This could be named titlefy instead of unslugify.

I like what you are trying to achieve, but I am not fond of the magic, I think there are tricky edge cases (off the top of my head, plus some I might not be thinking about).
More importantly, now slugs become first class citizens, and I don't want to make that commitment. Especially because I don't understand the problem we are solving enough to make possibly costly (in terms of edge cases and maintenance) decisions around that.

Just to share with you, here are my first questions:

  • Where would this logic apply? Just when creating a note from a placeholder or everywhere we use FOAM_TITLE (e.g. do we apply this logic to the title input by the user)?
  • What about a daily note with title 2021-12-16? Will it replace the dashes with spaces?
  • What if the user wants the slug as well as the title? FOAM_TITLE and FOAM_TITLE_SLUG? Can we guarantee that FOAM_TITLE = unslug(placeholder) and slug(FOAM_TITLE) === placeholder?
  • Could this break the mapping of wikilink to filename (e.g. if it uses FOAM_TITLE in the path)?

It's not about the answers here per se, but having to consider all of these (and what else?) that makes me uncomfortable with moving forward with this at this stage.

At the same time I do think your heuristic is pretty good, and probably many users will appreciate it.

So, I would shelve implementing the idea for now, and write a proposal about "Slugs in foam" to get the conversation started (also because I really don't know how many people actually care, and my guess is "not many").

@memeplex
Copy link
Contributor Author

Ok, once you merge #865 I'm going to adapt this PR to provide a simple slugify implementation and rebase on top of the updated master.

@riccardoferretti
Copy link
Collaborator

For now let's remove the slugger as it's not currently used. When we decide to introduce a feature that uses it, like #865, we will consider how to reintroduce it with a simple slugify implementation as you suggest

@riccardoferretti riccardoferretti merged commit 6073dc2 into master Dec 21, 2021
@riccardoferretti riccardoferretti deleted the remove-github-slugs branch December 21, 2021 20:32
@memeplex
Copy link
Contributor Author

We should update yarn.lock.

@riccardoferretti
Copy link
Collaborator

damn, I missed that, will do now

@memeplex
Copy link
Contributor Author

I'm pushing a PR right now.

@zomars
Copy link
Contributor

zomars commented Jul 26, 2022

Why this feature got removed? I got many slugified wiki links on my 3-year-old notes and now found out that almost all my links we're broken. Related to #1033 #1035 #1023

@riccardoferretti
Copy link
Collaborator

Foam was originally using Markdown Notes for wikilink resolution, which will slugify the link.
You can still use your older note base with Markdown Notes, but Foam has moved away from slugifying them.

@zomars
Copy link
Contributor

zomars commented Jul 26, 2022

Foam has moved away from slugifying them.

Why tho? Is that hard to give an explanation on the motives? Other than "it's harder to maintain". I don't think is that unreasonable to expect backwards compatibility. Even for those who already invested way to much time and effort into the foam ecosystem.

@riccardoferretti
Copy link
Collaborator

I have seen you already found it, but in case it's helpful for others more information on the rationale for moving away from slugification is found here: #1033

@zomars
Copy link
Contributor

zomars commented Jul 27, 2022

There's no rationale there. Only a "we're moving away from slugified links without giving reasons other than a moved file bug". Can you please help me understand the why? I personally don't see the benefit and would need to look alternatives to keep my links usable. We're talking about hundred of links rendered unusable. Or maybe allow us to downgrade to a slugified working version.

@memeplex
Copy link
Contributor Author

Why? Because as in every other project, there are limited resources here, complexity creeps and the slug part was particularly messy with a legacy of not very consistent code and criteria. As you can see I've proposed a compromise version above so that some use cases could be preserved, but anyway I tend to agree with Ricardo here, not least because he is the one doing most of the heavy lifting. Perhaps you can write a simple script that deslugify your files or work in a PR that may or may not be accepted (probably not if you just hack some code in order to support your and just your use case). After all, you're using code that's not only free but also:

image

which is clearly stated in the project github home.

@zomars
Copy link
Contributor

zomars commented Jul 27, 2022

I'm aware of that @memeplex I'm an open source contributor myself. But please try to emphasize a little bit with my story. I started using foam from almost day one. Made hundreds and hundreds of notes with it. Fast forward a year and tried to retake my writings just to find out that all my links are broken. I'm not expecting anything but some empathy here, please try to understand my frustration. I'm just trying to recover my links and without luck in finding a clear way to do so.

@memeplex
Copy link
Contributor Author

Is it that difficult to deslugify your wiki? You can contact me if you need help doing that.

@riccardoferretti
Copy link
Collaborator

hi @zomars, I hear what you say. When this choice was made I was aware that some people were going to be affected. And yes in those days Foam was really alpha! (it still kinda is, yet we haven't released the API exactly because even when we say we are alpha users still then expect the API being backwards compatible)

Inspired by #1033, let's go back to the reasons why I decided to make the switch (which I was expecting to create some disagreements, but I thought the tradeoffs were still positive):

  • slugification is not unique: If multiple wikilink strings can reference the same slug, (e.g. [[my file-name]], [[my-file name]], [[my-file-name]], all become my-file-name.md, [[asp.net]] and [[aspnet]] are slugified to aspent.md, and so on..) trying to de-slugify quickly becomes complex
  • some people say this case is not super common, but unfortunately the code has to work in most cases, which means that we would have likely needed to account for it, even if accounting meant breaking
  • adding a level of indirection, a slugification function, between the text of the wikilink and the corresponding file name creates complexity for other features and use cases
    • e.g. we added refactoring support and I am not sure how much harder it would have been with slugification, but I am sure it would have not been easier and I am glad I didn't have to think of the various edge cases involved
  • we introduced the concept of "Foam identifier", which is not strictly markdown but allows people that have files with the same name across folders to uniquely identify them - again, I am glad I didn't have to think about slugification, or even worse a configuration option to switch between slugified or not links
  • An additional level of complexity is due to several slugification functions incompatible with each other (we have already dealt with this problem in Foam for markdown-notes).
    • we actually had a couple of issues due to incompatible slugification back in the days
  • Slugification is also unfriendly towards non-latin alphabets and we have users that don't use them

Beside all of those reasons, from a design POV I simply don't believe that slugs are a good foundation, and that's why I chose to get rid of them. I might be wrong, by all means, I have just not been persuaded by the arguments yet.

At the same time we had some features in the works that would have softened the blow for people that really like to use slugified filenames:

  • [[my-file|My File]]: Foam today supports labels
  • FOAM_SLUG: Foam today supports this variable to access a slugified version of the title (which you can use in your template as the filepath)
  • alias: My File: Foam today supports the alias property which can be used to achieve the [[my-file|My File]] link directly with autocompletion
  • using link definitions [[My File]] and [My File]: my-file.md - not supported today by Foam (and no plans to implement it in Foam core), but it could make it easier for a third party to build on the Foam model and achieve (explicit) slugification, without having labels in the links

If we find a clean solution to supporting slugification we might add it, by all means.
What I mean by "clean" is simple: I don't want the core model to have to deal with slugs. In my view slugification is a feature, not a design choice, not a building block. The very conversation in this PR (pre-merge) shows how tricky things can quickly become when slugs are foundational.

With the building blocks we currently have:

  • templates
  • FOAM_SLUG and FOAM_TITLE variables
  • alias property

We are getting very close to have an almost seamless support for slugification (if you are happy with all your links looking like [[my-file|My File]], hence "almost seamless").
We are about to add another building block, which I think will make things even easier, currently captured in #1038.

And maybe in the future we'll have link definitions (my last point above) as a great way to support "non-aliased" slugs links in Foam, but that is not a priority in my opinion at this point.

I don't expect everyone to agree with the design choice, and I do appreciate the feedback (when done constructively), slug vs non-slug has been an ongoing topic, and I do want to build a solution that in the end works for the majority of users.

Back to your situation, here are a few alternatives to keep using the old-fashioned slugs:

  • use an earlier version of Foam
  • use Markdown Notes
  • and of course, as @memeplex was saying, write a script to slugify the links or deslugify the filenames (if the API were released it would be trivial thanks to the Foam model, but unfortunately that has not been released yet)

@zomars
Copy link
Contributor

zomars commented Jul 27, 2022

How do you deal with publishing your notes on the web? Most of the early day examples and templates were heavily focused on publishing to the web. Which is tied hand-to-hand with suglification. I know because I've contributed to eleventy and next.js examples myself.

@zomars
Copy link
Contributor

zomars commented Jul 27, 2022

I appreciate the explanation tho. How could tell VS Code to use a previous foam version?

@riccardoferretti
Copy link
Collaborator

riccardoferretti commented Jul 27, 2022

Regarding the website, you just need to use [[my-file]] or [[my-file|My File]] links.

Re installing a different version, click on the gear in the Foam row:
image

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

Successfully merging this pull request may close these issues.

3 participants