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

feat: provide MediaQuery / prefersReducedMotion #14422

Merged
merged 25 commits into from
Dec 5, 2024
Merged

feat: provide MediaQuery / prefersReducedMotion #14422

merged 25 commits into from
Dec 5, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Nov 25, 2024

closes #5346

TODO:

  • is this the right API? -> mostly, but matches should be current
  • provide prefersReducedMotion or not? (it's kinda trivial to reproduce in userland, but also doesn't hurt to provide / makes for better DX than to always repeat it in userland) -> yes
  • should the start stop notifier logic be extracted into a public function you can import from svelte/reactivity? It was asked for several times AFAIK
    • yes; made a start, does the API / name make sense?
  • changeset + docs

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Nov 25, 2024

🦋 Changeset detected

Latest commit: f2406b2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14422-svelte.vercel.app/

this is an automated message

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@14422

@Rich-Harris
Copy link
Member

Wondering if createSubscriber currently has the right API. In practice you're always going to need to do some version of the #version dance, which suggests it should be part of the API:

```diff
export function createSubscriber(start) {
+ let version = source(0);
  let subscribers = 0;
  /** @type {(() => void) | void} */
  let stop;

  return () => {
    if (effect_tracking()) {
+     get(version);

      render_effect(() => {
        if (subscribers === 0) {
-         stop = untrack(start);
+         stop = untrack(() => start(() => increment(version)));
        }

        subscribers += 1;

        return () => {
          tick().then(() => {
            // Only count down after timeout, else we would reach 0 before our own render effect reruns,
            // but reach 1 again when the tick callback of the prior teardown runs. That would mean we
            // re-subcribe unnecessarily and create a memory leak because the old subscription is never cleaned up.
            subscribers -= 1;

            if (subscribers === 0) {
              stop?.();
              stop = undefined;
            }
          });
        };
      });
    }
  };
}

On the consumer side, life gets slightly simpler:

```diff
export class MediaQuery {
  #query;
- #version = source(0);
- #subscribe = createSubscriber(() => {
-   return on(this.#query, 'change', () => increment(this.#version));
+ #subscribe = createSubscriber((update) => {
+   return on(this.#query, 'change', update);
  });

  get current() {
-   if (effect_tracking()) {
-     get(this.#version);
-     this.#subscribe();
-   }
+   this.#subscribe();

    return this.#query.matches;
  }

  /**
   * @param {string} query A media query string
   * @param {boolean} [matches] Fallback value for the server
   */
  constructor(query, matches) {
    // For convenience (and because people likely forget them) we add the parentheses; double parantheses are not a problem
    this.#query = window.matchMedia(`(${query})`);
  }
}

I suspect it would also make code more robust. Without the update callback, it feels like I'm supposed to do something inside whatever event handler I create inside the start function. Most likely, I'm going to update some local state that then gets returned from my get current function...

class Elapsed {
  #start = Date.now();
  #now = $state(Date.now());

  #subscribe = createSubscriber(() => {
    const interval = setInterval(() => this.#now = Date.now(), 1000);
    return () => clearInterval(interval);
  });

  get current() {
    this.#subscribe();
    return Math.round((this.#now - this.#start) / 1000);
  }
}

...which is incorrect, because this.#now will never be updated unless current happens to be read in a reactive context.

If version is managed inside createSubscriber, it becomes much more obvious how to write this code correctly:

class Elapsed {
  #start = Date.now();

  #subscribe = createSubscriber((update) => {
    const interval = setInterval(update, 1000);
    return () => clearInterval(interval);
  });

  get current() {
    this.#subscribe();
    return Math.round((Date.now() - this.#start) / 1000);
  }
}

It's also simpler. Are there cases where we do want to be able to set state from inside start, or does this change make sense?

@dummdidumm
Copy link
Member Author

Agree with your reasoning. I was initially against it because store/mediaquery seemed slightly different in how they're setup, but they turn out to be almost the same. Furthermore, if for some reason the API doesn't fit your use case you could choose to just not call update and instead manage your own signal.

@huntabyte
Copy link
Member

Should MediaQuery also accept a getter as a prop for reactive updates?

Similar to what we've done in https://runed.dev/docs/utilities/media-query

@Rich-Harris
Copy link
Member

Personally I think it's better to just do this:

let width = $state(640);

const large = $derived(new MediaQuery(`min-width: ${width}px`));

Occasionally it will result in a tiny bit more boilerplate. But it makes the implementation of MediaQuery (and similar classes) simpler, and creates a habit of combining basic primitives in a way that extends to classes that don't implement the 'if function then this, otherwise that' way of doing things. I'm very sceptical of overloads in general.

@TGlide
Copy link
Member

TGlide commented Dec 3, 2024

Occasionally it will result in a tiny bit more boilerplate. But it makes the implementation of MediaQuery (and similar classes) simpler, and creates a habit of combining basic primitives in a way that extends to classes that don't implement the 'if function then this, otherwise that' way of doing things. I'm very sceptical of overloads in general.

For complex utilities, won't this be worse for performance?

I've not been using overloads for this kind of prop, and it works fine I'd say. Its not as simple an implementation, admittedly.

// Tabs.svelte.ts

/**
 * Extracts the value from a getter or a value.
 * Optionally, a default value can be provided.
 */
function extract<T, D extends T>(
	value: MaybeGetter<T>,
	defaultValue?: D,
): D extends undefined | null ? T : Exclude<T, undefined | null> | D {
	if (isFunction(value)) {
		const getter = value;
		const gotten = getter();
		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		return (gotten ?? defaultValue ?? gotten) as any;
	}

	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	return (value ?? defaultValue ?? value) as any;
}

export class Tabs<T extends string = string> {
	/* Props */
	#props!: TabsProps<T>;
	readonly loop = $derived(extract(this.#props.loop, true));

	constructor(props: TabsProps<T> = {}) {
		this.#props = props;
		// ...
	}
}

Usage

const tabs = new Tabs<TabId>({ loop: () => controls.loop });

I don't even think the syntax is that much better or anything like that, but for my use case, I think re-instantiating it all would be quite expensive. Wouldn't that be the case potentially for one of Svelte's utils?

@Rich-Harris
Copy link
Member

If it's expensive to recreate, then it was probably too expensive to create. On a case-by-case basis it may be worthwhile, but in my experience doing extra work in order to do less work rarely pans out — the simpler thing is usually the right thing.

@TGlide
Copy link
Member

TGlide commented Dec 3, 2024

If it's expensive to _re_create, then it was probably too expensive to create. On a case-by-case basis it may be worthwhile, but in my experience doing extra work in order to do less work rarely pans out — the simpler thing is usually the right thing.

It's not only a matter of expensiveness, but also keeping internal state intact. For MediaQuery there's none, but wouldn't other utils have some?

I have the worry that if certain cases arise where it is worthwhile, then it'll become inconsistent.

@Rich-Harris
Copy link
Member

Do you have specific examples in mind?

@TGlide
Copy link
Member

TGlide commented Dec 4, 2024

Do you have specific examples in mind?

Something like this:

const spring = new Spring(initialValue, opts)

How do I pass in reactive opts? If I use $derived, the internal value gets overwritten every time an option changes

@Rich-Harris
Copy link
Member

You don't, you interact with the spring directly:

spring.stiffness = 0.5;

If you need to sync it with some external source (not that I've ever needed to) then you can use an effect, making that rare case slightly dirtier in exchange for the common case being simpler

@TGlide
Copy link
Member

TGlide commented Dec 4, 2024

This becomes a bit inconsistent though, using effect is some places, and derived in others.

EOD, this is probably not a big deal, and I can see it makes sense for the utils Svelte provides.

I do think the community patterns will be different from this, but it may be a bias towards what I've been doing

@Rich-Harris
Copy link
Member

What we definitely don't want is to create confusion about the source of truth. Suppose I do have some reactive options:

let opts = $state({ stiffness: 0.15, damping: 0.8 });

let s1 = new Spring(0, () => opts);
let s2 = new Spring(0, () => opts);

Great! They're both controlled from the same place. But what happens when I do this?

s1.stiffness = 0.2;

Does that also affect s2.stiffness, via opts.stiffness? Presumably not since we had the () => (after all, it could be a $state.raw or a $derived). But what if we hadn't, and had just relied on the fact that it's a reactive object?

let s1 = new Spring(0, opts);
let s2 = new Spring(0, opts);

Does that work, even though typeof opts !== 'function'? Or is it treated just as an initializer? The cognitive overhead is already mounting.

Maybe we say that in the case where the options are determined to be reactive, s1.stiffness is readonly. But that seems confusing and restrictive, and you can't really express that in the types, at least not without cumbersome overloads. Or maybe we just don't expose s1.stiffness at all, and if you want to change it then you have to use reactive options, though of course that makes the common case more cumbersome.

In other words: the added complexity isn't confined to the implementation! It spreads around. I totally get the motivation — lord knows I've written my fair share of these convenience don't-make-me-think APIs — but in my experience being overly accommodating about what inputs you accept forces users to think much more. It's definitely not how I envisage our APIs evolving.

@TGlide
Copy link
Member

TGlide commented Dec 4, 2024

I've definitely faced the problems you described, and its a bit of a head scratcher.

For Melt, I've gone with s1.stiffness is always readonly. It is a bit restrictive indeed, but it aligns with how people usually use the library, they instantiate components, and pass props down. Usage is as simple as const tabs = new Tabs({onValueChange, ...getters(props)})

I don't mind it too much, because it behaves mostly like a component, where you pass stuff down, and need to setup a state variable outside of it if you want to update it post-mount. I'd like for tabs.loop = false to be possible, but then I'd require an onLoopChange, or overloads, or some other hideous API. I also definitely don't want that!

But I don't think I should do const tabs = $derived(new Tabs(...props)), specially if props maybe contains value. Seems unnecessary to re-instantiate everything inside it, and I'd also lose all of the internal state that isn't shared with props. E.g. if props didn't contain value, or if Tabs had a private #focused field.

$effect is definitely less abrasive in this sense, but much more cumbersome.

const spring = new Spring(props)
$effect(() => {
  spring.stiffness = props.stiffness
  spring.optionTwo = props.optionTwo
  // ... do this for every prop. Alternatively, iterate with a loop over the keys.
})

And value is more complicated, because of two-way binding. With props, you probably don't need it, I don't see you doing spring.stiffness = hardcoded if you're syncing with an outside object. But its more common with value. How would you handle this?

Would this (Playground link) be the way to do it?


I'm sorry if this is not too pertinent for the matter at hand, since I'm using Melt as an example, rather than an example util more suited to what Svelte would ship. I'm sharing mostly because I've had my troubles when trying to create a good consumer API for Melt and Runed, and I think the community has been iterating as well to try and find what they like best, so seeing how Svelte will shape its own utils is important to me.

@Rich-Harris Rich-Harris marked this pull request as ready for review December 5, 2024 15:24
@Rich-Harris Rich-Harris merged commit 0a9890b into main Dec 5, 2024
11 checks passed
@Rich-Harris Rich-Harris deleted the media-query branch December 5, 2024 15:24
@github-actions github-actions bot mentioned this pull request Dec 5, 2024
@Rich-Harris
Copy link
Member

It is a bit restrictive indeed, but it aligns with how people usually use the library, they instantiate components, and pass props down

Yeah, I can see how the relative priority of different use cases might change if you're building stuff for component libraries vs primitives designed to be used directly by app authors

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.

Built-in support for respecting the prefers-reduced-motion flag with animations/transitions
5 participants