Export frontmatter attributes #9
Conversation
|
As a side note, I at first wasn't sure why this module bundles |
I actually had a note here that we should do exactly what you're describing: https://github.com/skipjack/remark-loader/blob/master/src/index.js#L15-L16 Added specifically for image handling and it felt janky to me at the time too. I think if we can factor it out safely, that is the "correct" approach. Disclaimer: I too have not done a ton of loader writing 😁 I will take a look at this PR in more detail tonight (EST). |
Having used my fork in a project a bit more, I've realized just having a single |
|
Okay, I've updated this PR so that frontmatter is assigned to an object exported as |
|
First off, this is great thanks for your work! Given how much is here, I don't have time to go through all this tonight but I will by the weekend if not sooner. Here's some initial thoughts...
|
There was a problem hiding this comment.
Thanks again for your work on this. I'm not using this project much anymore and you may have noticed that @evilebottnawi and I just transferred it into the webpack-contrib organization (#6). With those things in mind, I'm going to leave this to the webpack team for the final decision but I did run this locally and left some thoughts below.
Hope this helps!
| Frontmatter values are assigned to an object exported as `attributes`. For | ||
| example, if you have this markdown file: | ||
|
|
||
| ```md | ||
| --- | ||
| title: a clever remark | ||
| --- | ||
| hello world! | ||
| ``` | ||
|
|
||
| You can use it like this: | ||
|
|
||
| ```js | ||
| import contents, { attributes } from './test.md' | ||
|
|
||
| console.log(contents) // logs "hello world!" | ||
| console.log(attributes) // logs "{ title: 'a clever remark' }" | ||
| ``` |
There was a problem hiding this comment.
Does remark parse frontmatter out of the box? I thought remark-frontmatter was required for this. If it is, then this bit feels a little funny (as do some of the tests) because this loader should really be agnostic of any plugins. Maybe a more generic exposure of of additional processed fields would make sense?
Otherwise -- if it does support frontmatter out of the box and no other processed fields -- then I would just simplify this down to something like:
If you're using frontmatter, you can
importit as a partial:import content, { attributes } from './test.md'
There was a problem hiding this comment.
I was going by what's already in the library, which already parses frontmatter via the front-matter package. Agreed that it could be better to leave that to userspace as a remark plugin! (That said, I think the use of frontmatter in markdown files is so common that it could be useful to include it as an opinionated default, similar to how remark-html is included now.)
There was a problem hiding this comment.
🤦♂️ wow that shows how long it's been since I touched this. Completely forgot that, I must've added that as a quick hack before I knew about the plug-in. Would definitely be good to get rid of that and somehow this via the plug-in instead. Just to clarify, there already is a frontmatter plugin for remark, I linked it above.
| "jest": "^26.1.0", | ||
| "memfs": "^3.2.0", | ||
| "standard-version": "^4.2.0", | ||
| "webpack": "^4.43.0" |
There was a problem hiding this comment.
I didn't dig in too much, but this branch yields a lot of audit warnings:
I submitted #10 to address most of the ones currently on master (~20).
There was a problem hiding this comment.
Sounds good -- I didn't upgrade any packages other than the ones needed for this PR.
There was a problem hiding this comment.
Yeah those additions may have vulnerabilities though, maybe try an npm audit fix on this branch and see if it knocks some out?
| parse(content, options) | ||
| // @todo we should probably just intercept images in the tree | ||
| // or recommend that the `html-loader` be chained |
There was a problem hiding this comment.
As noted in my previous comment, I would keep the @todo unless it's really impossible or impractical for us to recommend chaining here.
There was a problem hiding this comment.
Agreed that it would be ideal to just leave this to webpack. As it stands, I couldn't figure out how to make this work, because this loader needs to intervene both before and after html-loader. Currently the loader passes its output to html-loader, receives the result, and then appends the frontmatter export to that result. Would love to hear if there's a smarter way to do this!
There was a problem hiding this comment.
Indeed, which may have been why I did it that way in the first place. I'll do a little research on this front and get back to you.
In the short term I would keep the todo and then maybe the best next steps would be to merge this as is and create an issue where we discuss a more robust refactor that allows for chaining and supports frontmatter via plugins rather than the current approach.
| @@ -19,19 +19,42 @@ | |||
| "main": "src/index.js", | |||
| "scripts": { | |||
| "lint": "eslint .", | |||
| "release": "standard-version" | |||
| "release": "standard-version", | |||
| "test": "jest" | |||
There was a problem hiding this comment.
I ran the test suite and everything passed, however it is throwing this warning at the end:
There was a problem hiding this comment.
Not sure about this -- the tests are somewhat cribbed from html-loader's, so maybe @evilebottnawi has more insight on what's happening. (Actually, it might be ideal if there were a shared loader testing library for webpack-contrib repos....?)
|
Why do you need this? I think it is out of scope this loader |
|
We did full refactor - using loader inside loader is bad idea, we split them, so you need to install |
|
@evilebottnawi Hi, it's still not clear to me with this refactor whether it's possible to take markdown with frontmatter attributes, and import it as an object that provides both the markdown converted to HTML as well as the frontmatter attributes. This is a very common use case -- in fact, I'd argue that by far the most common use case of markdown with frontmatter is to convert it to HTML and an object of frontmatter attributes. That is the use case this PR was meant to address. Generally when one supplies frontmatter in a markdown file, it is because they need access to this metadata -- in the general case I don't think people want to just throw away the frontmatter. I understand that when webpack took over ownership of this repo, they decided to ignore all pending PRs and simply rewrite the package, but it's pretty disappointing to me that they wouldn't consider IMO what's an actual significant use case. As far as I can tell, given the way the package exist now, if I wanted to handle this use case of HTML and frontmatter, I'd need to fork Am I incorrect? Is there a simple way using this loader to convert a markdown file with frontmatter such that I can import both HTML output and frontmatter attributes? |
It's a shame to hear, because I asked you before the release, but you showed no activity, so I figured that we can figure it out later. I am a bit confused about what you want to solve exporting attributes? I would say more - it can potentially break some cases. If you presented short example/repo with readme what you are trying to achieve, we would be able to find a better and more convenient solution for you, without this context, I cannot just take and start export something, tomorrow somebody can ask export full ast, then something else, and you bundle will be big and slow |
I'm sorry if that's the case, but I'm not sure it is? I think both @skipjack and I pinged you in this PR a month ago, but it looks you only replied to this thread about two hours before the release was cut. I'm sorry if I wasn't able to jump on the issue to reply that quickly.
That seems like a kind of absurd counterexample? I don't think frontmatter attributes are irrelevant metadata or some implementation detail like an AST. For example, most of the popular tooling that consumes markdown files explicitly makes use of frontmatter. Jekyll (used by default for Github Pages) employs it to determine layout template as well as variables that can be interpolated into templates; Gatsby allows querying it; Hugo makes extensive use of it as well. Webpack's own website, webpack.js.org, makes extensive use of frontmatter attributes in its markdown files. I feel it's very likely that if people start to use this loader more extensively, this issue will be raised again, because in my opinion it's a significant one. I don't have a full public repo with an example of this, but as a very short sketch here's how usage of frontmatter attributes as proposed by this PR could work: // ...
const { default: content, attributes } = await import(`content/${page}/${lang}.md`);
const { title, publishDate, published } = attributes;
// render a page statically using the given html body `content`, interpolating
// the title and publication date into the template, predicated on whether the
// `published` content attribute is true/false or undefined.I can try extract a more extensive example (rendering to static HTML with React via Next.js, since that's the context I'm currently using it in) if that would be helpful or more illustrative of why frontmatter is valuable. |
Hey @lostfictions, this is at least partially on me (since I haven't had time for the repo recently) as well as just some unlucky timing during the transition. You definitely got burned here and I'm sorry about that, but I think we can still come to some sort of resolution for what you're proposing. I agree the use case you were shooting for is common and what you outlined is clear. I think there's probably a good way to support it we just have to think of how to pass an additional export through the next loader in the chain. @evilebottnawi @sokra @bebraw is that even possible? @lostfictions I think the most logical next step is to create an issue with what you outlined above so we can brainstorm more there. Unfortunately this pr should probably be closed at this point but you can always keep your fork and install it through npm's GitHub url feature. |
|
@lostfictions remark loader do not export JS, you can look at source code, but we can |
|
A loader can return multiple values. The first one is usually the content, that would be HTML here. The second one is usually a SourceMap, not sure if this is available here but we should reserve it for that at least. The 3rd one is usually an options object with more things. You could put an It's also possible without html-loader modifications, but this would require an additional loader which adds the additional export after js generation. This is less UX friendly and more complex, so better go with the first approach. |
|
Those details are exactly what we needed and resolve the discussion we started having on the initial changes in this pull request. @lostfictions what @sokra laid out above is exactly what we need...
I think the path forward is fairly obvious now. Let's create two issues...
The complete picture won't work until both changes are in place but they should be able to be addressed in parallel and then tied together. Again sorry this PR didn't make it through, but once we flesh out the details in those issues my guess is that @evilebottnawi and the rest of the team would welcome more targeted PRs to address those two steps and provide support for what you described. |
|
@skipjack Thanks for following up on this! And sorry for my frustration that this PR was passed over -- I totally get that it was leaning on the pre-rewrite hackish approach of this loader, and that modifying html-loader and this loader to support some kind of handover as you and Tobias outlined makes more sense. I'm not sure I have the capacity to write up these issues (or take a pass at the necessary PRs) in the next little while -- for better or for worse my hackish fork is working well for the project I needed it for -- but if nobody else gets to it sooner I could maybe take a look in a few weeks. |
|
No worries, let me know if you'd like any help creating them. Please cc me on them and link/reference this pr if you do. |
|
@lostfictions I can help, it is not hard, please open a new issue, otherwise we will lost the issue |
I noticed that frontmatter from the markdown is being extracted, but discarded. This PR fixes that!
To accomplish this, I've upgraded
html-loaderto the latest version, so that we can pass in theesModuleoption. Without this,html-loaderreturns something likemodule.exports='<html>...', which makes our job a lot more difficult.With ES Modules, we can continue to default export the HTML, but also append named exports for each attribute. (I've opted for individual named exports here assuming this is better for tree-shakeability, but we could also, say, export a single value named
attributescontaining all the frontmatter values.)As a bonus, this loader also can now accept a
htmlLoaderOptionsoption to forward tohtml-loader. I've added all this to the readme.Your lint configuration also didn't seem to be working; I've fixed that.