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

Support exclusive lists with GoToSocial 0.17 #817

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

graue
Copy link
Contributor

@graue graue commented Sep 19, 2024

Fixes #808: supports exclusive lists with GoToSocial 0.17+.

Unlike some other server types, GTS does not use version strings that pretend to be Mastodon (for example, Pixelfed's version string of "3.5.3 (compatible; Pixelfed 0.12.3)". Instead, to detect that the server is GTS, we must use NodeInfo. I've added support for this and rewritten the feature detection to use it.

Note that for Mastodon <4.3 (all current stable releases of Mastodon), the NodeInfo request will fail due to mastodon/mastodon#23135, so we'll fall back to the existing behavior. Mastodon 4.3 fixes this. For other server software, this will allow for more accurate checking of feature availability.

@cheeaun
Copy link
Owner

cheeaun commented Sep 19, 2024

Honestly, this changes too many things at once, which makes it hard for me to review.

Few notes:

  • Could be simplified by just introducing @gotosocial/list-exclusive.
  • @mastodon/list-exclusive': [['mastodon', '>=4.2'], ['gotosocial', '>=0.17']], looks confusing to me, thus the point above.
  • versionSatisfies(software, version, [softwareSpec, versionSpec]) is kinda unreadable to me
  • I'll still prefer that current implementation is untouched and add nodeinfo check only for platforms that don't add their own user agent with Mastodon's, for now. Try not to over-engineer this as there could be many more platforms in the future that might do different things, especially learning from the history of browser user agent sniffing 🙈

@graue
Copy link
Contributor Author

graue commented Sep 20, 2024

Thanks, yeah, that part could definitely be written better. I can take a second pass on readability and split it into multiple commits, although it also sounds like you don't like this approach at all...

Could be simplified by just introducing @gotosocial/list-exclusive.

I thought you might prefer only to have to check one flag at the point where the feature is implemented. This could one day also become @akkoma/list-exclusive, @pixelfed/list-exclusive...

I'll still prefer that current implementation is untouched and add nodeinfo check only for platforms that don't add their own user agent with Mastodon's, for now.

The issue here is, without getting the nodeinfo, Phanpy doesn't know it's a platform that doesn't add its own user agent with Mastodon's. Unless the code did something like assume that a version >=3 is Mastodon and <3 is GTS, but that doesn't seem very future-proof.

Nodeinfo has become a widely used standard, with this list of implementations including every kind of server Phanpy has issues about (except one, you have an issue tagged Takahe which isn't in the list but I checked and it also supports nodeinfo).

@graue
Copy link
Contributor Author

graue commented Sep 20, 2024

All right, here's a smaller version of the change. It feels a bit less future-proof to me than what I was trying to do, but I admit my implementation was awkward. Perhaps if projects start using the NodeInfo more heavily to communicate their capabilities, this could be revisited later.

@graue graue changed the title Use NodeInfo to detect features if available Support exclusive lists with GoToSocial 0.17 Sep 20, 2024
@cheeaun cheeaun added enhancement New feature or request gotosocial labels Sep 22, 2024
@graue
Copy link
Contributor Author

graue commented Oct 9, 2024

@cheeaun, any thoughts on my revised version of this based on your feedback?

For what it's worth (side note...), with Mastodon 4.3 out now, the nodeInfo API will become almost universally supported as servers upgrade.

src/utils/api.js Outdated Show resolved Hide resolved
src/utils/supports.js Outdated Show resolved Hide resolved
loose: true,
}));
return (supportsCache[key] = (
containGTS.test(feature) === containGTS.test(software_name)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is honestly a bit hard to read. I get it, if it's Mastodon, it'll be false === false which returns true 😬

Could possibly prepend this above instead? e.g.:

if (softwareName.toLowerCase() === 'gotosocial' && feature.startsWith('@gotosocial') {
  return satisfies(version, range, {
    includePrerelease: true,
    loose: true,
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that doesn't quite work, because if the feature is GoToSocial but the server is Mastodon, or vice versa, and if it then falls through to the original satisfies check, it's going to give incorrect results. We need some notion of "Does the software that's running on this server match the software this feature applies to?"

For Mastodon <=4.3 (all current stable releases of Mastodon), the
NodeInfo request will always fail due to mastodon/mastodon#23135 and
fall back to the existing behavior. For other server software, this will
allow for more accurate checking of feature availability.

Fixes cheeaun#808: adds support for exclusive lists with GoToSocial 0.17+.
@graue
Copy link
Contributor Author

graue commented Oct 11, 2024

Rebased and addressed feedback. Hopefully this will do the trick for you!

(Note: the line treating Hometown as Mastodon future-proofs against a Mastodon 4.3-based release of Hometown, in which the node info API will start working and setting 'hometown' as the softwareName.)

}));

// '@mastodon/blah' => 'mastodon'
const featureSoftware = feature.match(/^@([a-z]+)\//)[1];
Copy link
Owner

@cheeaun cheeaun Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be like (feature.match(/^@([a-z]+)\//) || [,false])[1] because if there's no match, match will return null.

I know my code above looks fancy (it's in other files too) so feel free to write it in a more understandable way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I could rely on supports() only being called with strings that match the @software/feature-name pattern; currently, this is the case:

$ git grep supports\(
src/components/account-info.jsx:                {supports('@mastodon/profile-private-note') && (
src/components/account-info.jsx:              supports('@mastodon/profile-edit') && (
src/components/compose.jsx:                if (editStatus && supports('@mastodon/edit-media-attributes')) {
src/components/compose.jsx:                {(supports('@pleroma/local-visibility-post') ||
src/components/compose.jsx:                  supports('@akkoma/local-visibility-post')) && (
src/components/compose.jsx:  const supportsEdit = supports('@mastodon/edit-media-attributes');
... etc., all of the calls are like this

So it seems like a call which did not match that pattern would likely be a bug. Do you want me to check for that here anyway?

let softwareName = getCurrentNodeInfo()?.software?.name || 'mastodon';

if (softwareName === 'hometown') {
// Hometown is a Mastodon fork and inherits its features
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering, how is this relevant to the current PR, which is meant for GTS?

Copy link
Contributor Author

@graue graue Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a future Hometown release includes the fix for nodeInfo, softwareName will be 'hometown', so for features like '@mastodon/list-exclusive', without the above line of code, this comparison would be false ('mastodon' === 'hometown'):

const doesSoftwareMatch = featureSoftware === softwareName.toLowerCase();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gotosocial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support exclusive lists with GotoSocial 0.17
2 participants