-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(experimental): authorship #292
base: main
Are you sure you want to change the base?
Conversation
result.url = urls[0]; | ||
} | ||
} else if (hCard) { | ||
if (URL.canParse(String(hCard))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can use URL.canParse()
- see https://developer.mozilla.org/en-US/docs/Web/API/URL/canParse_static#browser_compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, think I confused myself with another Node-only project supporting Node v20. I’ll create a utility function that achieves similar.
@paulrobertlloyd thanks for taking the time to put this proposal together. As an experimental option, provided the implementation is faily lightweight (e.g. no additional dependencies), I'm happy to include it with this package. I really appreciate the tests, this is the most important thing to get in so we can work towards the feature working. It's been a great help in sorting out the issues you've experienced. I've managed to get the tests working, and did a little refactoring. These are all available in this commit: 50aa892. I've had to comment out a step or two as they were not needed to get to this point. If you're happy to continue, it should be in a state where you can work on the last 3 tests that are failing. As for TypeScript - it's far from uncommon for open source javascript packages to be written in TypeScript - take parse5, which is the only dependency this package uses, and rollup, which we use to bundle this, are themselves built with TypeScript. I do appreciate that it may make it easier for some people to contribute if I was to drop TypeScript, but on the other hand it would also make it significantly harder for me to maintain. I am actively maintaining this, so I am around to help out if anyone needs any help contributing. |
Thanks for reviewing and refactoring this PR. Do you know how I can add your commit to my branch? I’m guessing using On getting those last 3 tests working, as mentioned, these will require an asynchronous fetch, which would in-turn require the entire function to be awaited, for example: const parsed = await mf2('<a class="h-card" href="https://another.example" rel="me">Jimmy</a>', {
baseUrl: "http://example.com/",
experimental: {
authorhship: true
}
}); I’m not sure you could create a function that is only asynchronous if certain options are enabled. What do you believe is the right way to include this optional fetch functionality? Are we happy for the API to be changed in this way (which would be a breaking change)? Also, with regards to fetch, are you happy to only support native fetch (which would require the minimum Node version being bumped to v18+, and this experimental feature not working in legacy browsers)? |
I have been thinking about this one for the last couple days since I saw this PR. The following are some random thoughts in no particular order.
You can. You could make type Wrapped<T> = { wrapped: T };
type Options = { baseUrl?: string, experimental?: { authorship?: boolean } };
function wrap<T>(input: T, options?: Options & { experimental?: { authorship?: false }}): Wrapped<T>;
function wrap<T>(input: T, options?: Options & { experimental?: { authorship: true }}): Promise<Wrapped<T>>;
function wrap<T>(input: T, options?: Options) {
const output = { wrapped: input };
return options?.experimental?.authorship ? Promise.resolve(output) : output;
}
const justAString = wrap("hi");
// ^?
const justAPromiseString = wrap("hi", { experimental: { authorship: true } } );
// ^? The With that said I am not sure how good of an idea that is in reality. |
I will comment about the thought I had when writing this parser. It was intentionally syncronous, and didn't provide any Using a TypeScript overload is possible, as pointed out, but I think this would be confusing. If you're not using TypeScript, you can get a promise, or not, returned based on an experimental feature toggle. If this library supports it, I would look to perhaps have it in it's own exported function, that returns a promise and that's explicitly set up for this. As for if authorship belongs in the same method as the parsing, I can't comment - with this I am very happy following spec or what is the community consensus for what it should be. |
Checklist
Changes to parsing behaviour
The Authorship specification is an algorithm for discovering and determining the author of a post. This algorithm is especially useful when implementing Webmention and wanting to infer the authorship of individual posts.
This PR is the start of a discussion about adding this algorithm, either to this project, or somewhere else in the Microformats + Node/JavaScript ecosystem.
Authorship discovery is currently an open feature request on
mf2tojf2
(see getindiekit/mf2tojf2#22). It would be useful when looking to develop downstream libraries such as webmention-handler (a project which currently implements it’s own authorship normalisation).There are a few options where authorship discovery and parsing could live:
mf2tojf2
and other downstream JF2 packages,webmention-handler
and other downstream packages that need authorship discovery.Example input covered by new behaviour
Tests have been added with example HTML, largely adapted from the validation examples provided on authorship.rocks.
Example output from new behaviour
Tests have been added with expected mf2 output. Currently these tests fail, but expectedly as the required discovery and parsing is not performed, even with the experimental
authorship
flag enabled.Other changes
Besides tests, and adding an
authorship
option to the experimental flags, I’ve added thesrc/helpers/authorship.ts
helper, whosefindAuthor
1 function is exported and used insrc/microformats/parse.ts
, where I think it might be called. Again, this is assuming this functionality is added as an experimental option.I’ve added comments that reflect my best understanding of the algorithm, and where code might live to implement it.
One reason why such parsing might want to be provided as a discrete function, is that later stages of the discovery algorithm require fetching remote content and parsing that to find a representative
h-card
. That would require making the entire function asynchronous.Anything else
‘I don't know TypeScript or my tests won't pass’ is a pretty good summary of my weekend.
I’ll try to refrain from adding a long and perhaps unhelpful rant about TypeScript, except to say that I’ve found it immensely difficult and frustrating trying to put together a useful enough PR, and have instead spent most of my time trying to understand various TypeScript errors and behaviours.
Perhaps a discussion for a separate PR, but I’d love to see this project move to model in which JSDoc type annotations are used (and exported as type definitions). Not only would this make it easier for people without TypeScript knowledge to contribute, it would remove the need for a build step and make the codebase more future proof and portable.
</rant>
Footnotes
This function is partly based on
find_author
provided bymf2utiljs
↩