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

[plugin-markdown] better route config options #61

Open
Apollinaire opened this issue Nov 17, 2020 · 7 comments
Open

[plugin-markdown] better route config options #61

Apollinaire opened this issue Nov 17, 2020 · 7 comments
Labels
good first issue Good for newcomers

Comments

@Apollinaire
Copy link
Contributor

The markdown plugin has a nice and simple way to configure how it deals with content, by just passing the name of the routes you want it to handle. However, this "magic" limits the devs on the way they organize the content inside their repository: they simply can't take their markdown files out of src/routes/[route]/ . This feels similar to Nextjs' routing system. It's not bad, but it has its limitations, and when for Nextjs it's justified by the fact that this is core to their build system, that they do a lot of optimizations on top of it, I think it's not justified for the markdown plugin.
I'd like to discuss the possibilities there are to change the config so that the plugin can handle markdown content anywhere inside the repository, and link it with a route.
Here is a proposal: the routes property can accept an array of objects of the following form:

const routes = [
  { content : [ "articles/travelling/*"], route: "travelling" }, // content is a list of globs
  { content : [ "articles/**/*.md", route: "allArticles"},
]

For backwards compatibility and ease of use, the current configuration can be kept as a shorthand for the following :

{ content: ["src/routes/travelling/*.md"], route: "travelling" }

Tell me what you think of this API, maybe improve it, and I can work on a PR

@nickreese
Copy link
Contributor

I like the idea.

I’d suggest we explore a plain object for the config where the key is the route name and the value is the array of content strings.

This will prevent having to parse the array and look for multiple definitions for the same route and simplify onboarding / documentation.

@Apollinaire
Copy link
Contributor Author

I like having an object to avoid deduplicating routes, however I think having an array of content strings limits the evolution of the config. It's easy to add a key to an object, it will never break other people's config if you make that key optional. If you just pass an array of strings, you can't add anything later without a breaking change, or at least some polymorphism like we are doing here. Of course you can always use another key than routes, but I think it's quite a good name.

@nickreese
Copy link
Contributor

@Apollinaire I'm with you on using an array. I think my usecase doesn't match the broader needs. Let me know how I can support.

@nickreese nickreese added the good first issue Good for newcomers label May 19, 2021
@noxasch
Copy link
Contributor

noxasch commented Jun 10, 2021

I like the idea.

I’d suggest we explore a plain object for the config where the key is the route name and the value is the array of content strings.

This will prevent having to parse the array and look for multiple definitions for the same route and simplify onboarding / documentation.

IMO this is a good idea as the route config allow us to separate the content and source code. So the config should look like this, where the route is as the content key if i follow it right as @nickreese said

{ 
  route: ['travelling', 'allArticles']
  content: { 
      travelling: "articles/travelling/*",
      allArticles: "articles/**/*.md"
  }
}

@nickreese
Copy link
Contributor

Would 100% accept a PR for this if you wanted to take a shot at it. Just haven’t had a chance to work on it.

I do like the structure you outline even if we need polymorphism in the future because it simplifies onboarding. We may want to document glob excludes so users don’t end up with duplicate content.

@noxasch
Copy link
Contributor

noxasch commented Jun 10, 2021

I'm not sure about allArticles use cases though, i just took it to show example of multiple route. But If the goals is to group all article into one, we can look for other method for that specific purpose like utilizing hooks or routes.

@nickreese
Copy link
Contributor

@noxasch 100% agree. I think it is bad practice to do an allArticles and was more taking it to mean that articles can live anywhere.

We could possibly track the files loaded and if files are duplicated throw an error about duplicate content.

As it currently stands all of the markdown is on the data object IIRC so in most cases it won't be needed.

noxasch added a commit to noxasch/plugins that referenced this issue Jun 13, 2021
- update index.spec.js

- reduce markdown input to bare minimum

address Elderjs#61
nickreese pushed a commit that referenced this issue Jun 15, 2021
* feat: add markdown content route config

- update index.spec.js

- reduce markdown input to bare minimum

address #61

* refactor: remove commented code

* docs: update plugin markdown docs
nickreese pushed a commit that referenced this issue Jul 30, 2021
* feat: add markdown content route config

- update index.spec.js

- reduce markdown input to bare minimum

address #61

* refactor: remove commented code

* docs: update plugin markdown docs

* feat: allow path/**/* usage for images src

- fix manifest not found when using **/*

close #67

* docs: update plugin-images docs

* fix(image-plugin): include plugin config

- resolves #119
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants