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

Switch to evergreen URL for wasm-feature-detect library #353

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

tomayac
Copy link
Collaborator

@tomayac tomayac commented Nov 2, 2023

Use https://unpkg.com/wasm-feature-detect@latest/dist/esm/index.js so we always include the latest build dynamically.

@andylizi
Copy link
Contributor

andylizi commented Nov 2, 2023

The library URL was originally set to use the latest semver-compatible version @1, but this was later changed in #288 (comment), so it's probably best to ask @RReverser to have a look here.

I personally think version pinning is unnecessary in our case, because #288 was purely caused by caching — the feature name in this project was updated, but some browsers were still loading the older library version. I don't feel this would be a problem because this can only happen when we are adding a new feature. It can never break existing features, because wasm-feature-detect cannot rename existing features without releasing 2.0, otherwise it'd be their fault for breaking semver.

So unless I'm missing something, the worst case scenario for using @1 is, we'd add a new feature, but the detection wouldn't be available to some users until their cache expire in 7 days, which is the max-age set by jsdelivr. Now admittedly 7 days is a bit long for my taste, however we can either switch to unpkg which only caches for 10 minutes, or we can use https://www.jsdelivr.com/tools/purge to manually purge the cache every time we add new features.


If not for the caching issue, it might be slightly better to use jsdelivr instead of unpkg, because the browser already has to connect to jsdelivr for another library:

website/roadmap.js

Lines 303 to 304 in 183bb19

// The ESM bundle of this package doesn't work with unpkg.com.
const module = import('https://cdn.jsdelivr.net/npm/@floating-ui/dom@1/+esm');

roadmap.md Outdated Show resolved Hide resolved
roadmap.js Outdated Show resolved Hide resolved
@tomayac
Copy link
Collaborator Author

tomayac commented Nov 2, 2023

No worries, switching back to jsdelivr, but now evergreen. What do you think?

@RReverser
Copy link
Member

So unless I'm missing something, the worst case scenario for using @1 is, we'd add a new feature, but the detection wouldn't be available to some users until their cache expire in 7 days, which is the max-age set by jsdelivr. Now admittedly 7 days is a bit long for my taste, however we can either switch to unpkg which only caches for 10 minutes, or we can use jsdelivr.com/tools/purge to manually purge the cache every time we add new features.

Correct, this was to make sure that users get up-to-date version. Basically same reason people add ?v=1 / hash / etc to JS URLs - to make sure that browsers update it and ignore the cached version.

We don't add features that often, and personally I think bumping version of wasm-feature-detect manually in the same PR when we add them is less of a hassle than having to purge cache elsewhere in CDN settings.

Even worse would be finding out that some users are broken when everything worked for you locally only after the deploy (like happened in #288), whereas a pinned version guarantees that your local browser has the same version of wasm-feature-detect as other users.

Hence I'm still strongly in favour of pinning dependency versions.

@andylizi
Copy link
Contributor

andylizi commented Nov 2, 2023

Even worse would be finding out that some users are broken when everything worked for you locally only after the deploy (like happened in #288)

How do you feel about switching to unpkg where the max-age is only 10 minutes?

Or even better, adding ?v=1 to the jsdelivr URL, so every time we want to make sure that users get up-to-date version, we can just increase v. This achieve the same benefit as version pinning, but with the additional benefit that, if someone forgot to change v, users can still get the latest version after a while. Sure, this can lead to temporary inconsistency if they forgot, but is it really worse than staying outdated for months if they forgot (see below)?

We don't add features that often, and personally I think bumping version of wasm-feature-detect manually in the same PR when we add them is less of a hassle than having to purge cache elsewhere in CDN settings.

This is uncommon in practice, because the detection support for a feature can often lag behind the website. E.g. when type reflections were added two years ago, it hadn't been supported in wasm-feature-detect, so there was nothing to bump.

Which is why I feel that, the approach of manually bumping versions currently relies on someone noticing the website is out of date and filing an issue/PR, which can take a long time.

Case in point, wasm-feature-detect 1.6 was released 6 days ago, adding support for multi-memory, type reflection, and JSPI, but I only noticed this because someone filed GoogleChromeLabs/wasm-feature-detect#73 after 6 days. And they, in turn, only discovered it because the GC detector happen to be broken in older versions + the latest Chrome stable is known to support GC, leading to a obvious discrepancy and a quick discovery. What if the GC detector wasn't broken? I'm worried it could take months before someone finally bump the version.

All in all, I don't really feel strongly about pinning the version or not, as long as we can find a way to avoid being out of date for long periods. After all, the whole purpose of the feature table is to provide up-to-date information. Using the latest URL is the simplest way I can think of, involving no extra bandwidth or manual work, which is why I advocate for it.

@tomayac
Copy link
Collaborator Author

tomayac commented Nov 2, 2023

As Wasm DevRel with 100 other things in my head, having this resource link be evergreen would mean one worry less. The library contains tests and a local website for manual checking, so as a newly appointed maintainer of the library (thanks, @surma), I commit to carefully checking before I release.

@RReverser
Copy link
Member

having this resource link be evergreen would mean one worry less

As described above and in previous issue, it actually adds one more worry about breaking part of the users. That's IMO worse than not having up-to-date checks if you forget a bump.

That said,

Or even better, adding ?v=1 to the jsdelivr URL, so every time we want to make sure that users get up-to-date version, we can just increase v. This achieve the same benefit as version pinning, but with the additional benefit that, if someone forgot to change v, users can still get the latest version after a while. Sure, this can lead to temporary inconsistency if they forgot, but is it really worse than staying outdated for months if they forgot (see below)?

This + unpkg sounds like a good alternative per reasons described in @andylizi's comment. Let's go with that.

roadmap.js Outdated Show resolved Hide resolved
roadmap.md Outdated Show resolved Hide resolved
@tomayac tomayac requested a review from andylizi November 3, 2023 19:58
Copy link
Contributor

@andylizi andylizi left a comment

Choose a reason for hiding this comment

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

After this update, every feature on the table should have their detection working! 🎉🎉

@dschuff dschuff merged commit d917d2d into WebAssembly:main Nov 3, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants