-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Decide how we want versioning via script embed to work #50
Comments
Case studies for Pinterest pin button, Facebook Like button Pinterest: https://developers.pinterest.com/tools/widget-builder/
Facebook: https://developers.facebook.com/docs/plugins/like-button/
|
I think allowing users to control the version is less developer-antagonistic; but for nonprofits who don't have the resources to spend time on this kinda tech stuff, i think the common case should be more like pinterest's flow of by default just using the latest version (barring breaking changes). |
The other thing I think would be useful, is to have some avenue for configuring the button on behalf of nonprofits, who once again may not want to spend the time setting all these config values; as we have a lot more parameters than either the facebook like/pinterest pin buttons; perhaps these aren't great examples of experiences to emulate, if what we're doing is something more complex, more akin to, say, the intercom/zendesk chat bubbles with branding options. |
Intercom, as an example, seems to do all the config on their website, and probably the embed calls back home to get that data; we can potentially do something similar. the approach I chose at the moment with our cloudflare worker was a client-specific URL, which I figured we could long-term inject configuration from the edge worker, leading to a more performant experience; but perhaps that's too standards-breaking/counterintuitive. https://www.intercom.com/help/en/articles/178-customize-the-intercom-messenger-basics |
Maybe we can just make these preconfigured URLs for nonprofits on a bespoke basis, but for the self-serve flow, just make the copy-paste flow have an optional version parameter that accepts something like standard semver ranges, or maybe just a major version; by default either not specify a version, or pin a major version. Re no version specified vs pinning, the upside of no version specified for us is that we can update existing clients for them. The downside of pinning a major version is that we can't force update clients if we go down that road; we can avoid breaking existing sites when a major version comes by still "remembering" the version we were serving, say, the domain that the request came from, trusting the Origin header; so that if we make a breaking change, we can still keep serving the older one until we've ensured it works on all the client websites/reached out to their teams. or we can just try hard to not make breaking changes, but given that this widget is in its infancy, i think we should be optimizing for making it powerful and polished over backwards compatibility for now. |
@jake-nz @markulrich what do you guys think? |
Agree ability for us to configure options for a given client is pretty important, especially as hopefully people will eventually be able to that from their Every.org admin dashboard - what do you think about https://github.com/everydotorg/every.org/issues/7297? As for a |
i'm gonna move that issue here since jake can't see it if it's in our main repo lol |
Posted by @markulrich : Today, we recommend people load script from URLs like https://assets.every.org/donate-button/CLIENT_ID/donate-button.js which I find a little confusing - can we instead make this an optional query parameter? So change to https://assets.every.org/donate-button.js?clientId=CLIENT_ID |
I would describe it rather - we in theory would recommend them that, but our I think the lack of copypastability of the API i chose tilts strongly in favor of that API - i'm happy to go with that! and we retain the potential for supporting specific version pinning if we add support for a version parameter. I do wonder why Facebook chose to put it in the hash instead of the query parameter, but to my knowledge i think query param should be fine. |
yeah idk why, it's pretty common for people to use query params to cache bust (for instance, stackoverflow does on its static js and css assets). I'm gonna assume that it's fine for now. So yeah lets go for a static URL that's just we should be careful about caching here - while we want people to load from cache for perf, we dont want caching to cause people to be stuck on old versions, so i'll make sure our caching policy's on cloudflare's side makes sense. once it's implemented we can make sure to edit the README to reflect this properly. I'll leave the logic for the existing URLs to keep working so that old clients dont break, but in the future, it would be a query param. |
one thing that might need to change from above is that we have a folder of stuff, not just a single js file, that does chunk splitting, so rather than Also, since there's only 5 registered slugs at the moment, we can avoid breaking those by just specifically hardcoding urls with those prefixes to follow the old path. |
If it's not too tricky, I think we could give developers 3 options:
Perhaps that last option isn't really needed though, 'Major version pinned' would probably serve that use case too |
Here's my suggestion for a simple but flexible experience for donate-button users, including versioning and config
Include the <script async defer crossorigin="anonymous" src="https://assets.every.org/donate-button/donate-button-2.js"></script> Add one or more donate buttons within your page. Show one-time donation flow
<a href="https://www.every.org/reeflegacy/donate?frequency=ONCE" data-every-button>
<img src="https://assets.every.org/donate-button/donate-button.png" />
</a>
Show monthly donation flow
<a href="https://www.every.org/reeflegacy/donate?frequency=MONTHLY" data-every-button>
<img src="https://assets.every.org/donate-button/donate-button.png" />
</a> For experts:
<script defer>
everyMonthWidget.setOptions({
... // See below for configuration options
})
</script> |
my thoughts on the above: Re doing it via static files - if we did that then the likely result is that developers would use Re granularity of specification - i think we should assume by default that nonprofits will not be updating their versions, most are not technical organizations and so I think we ought to operate under the assumption that the typical implementation will basically be set in stone for a very long time. for that reason I think we should push for just with that in mind i'd prefer just exporting a Re donate-button being an |
i also realized now that we can just make it |
Sounds good, I think we're basically on the same page here. Versioning Serving
|
yeah, now that i think of it i think that makes sense jake! I still think we'd want the cloudflare worker to basically alias semver versions to specific bundles instead of including them in the github repo, though maybe we can make it work with symbolic links? not sure if github pages supports that, will look into it! |
symlinks dont work lol. but we can still do the worker |
Yeah, good point. We don't really want all those in the repo |
So yeah after having played around with the code a bit more, we can do all this kinda semver features, but there needs to be other work first before we can do that dynamically, so I think we should keep it simple for now and not do it to that much detail. I think we should only support major versions specification for now, and just manually update the available versions. so routes like The main issue with more sophisticated semver matching is that the cloudflare worker needs to know what semver versions exist on Github Pages, so if you specified One way this could be done is by generating the list of versions and saving it as a JSON file in the workers code on deploy; another is to post the current list of versions to Cloudflare KV on deploy; alternatively, we could just generate a file that lists out the versions when Github Pages deploys like discussed here, and routinely cache that list so that we don't have to fetch it every time a request comes in. a couple other changes that would make the implementation simpler:
|
Thought a bit more, I think that semver is gonna be rough to support through URLs when you take into account caching - the client might expect an older minor/patch version, and if all the files aren't being served from the same directory, then it will cause failures on older clients. I have come around to just serving with all the files in directories with the major version in the name (PR at #65 ), and perhaps only doing something with the cloudflare worker for custom client bundles if necessary, but no longer mentioning that. |
we went for |
After doing #49 , let's decide where it should go. Right now no way for user to set the version they're using, it's totally in our control - we should potentially expose that
The text was updated successfully, but these errors were encountered: