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

Bring back m.prop? #2095

Closed
gilbert opened this issue Feb 28, 2018 · 13 comments · Fixed by #2268
Closed

Bring back m.prop? #2095

gilbert opened this issue Feb 28, 2018 · 13 comments · Fixed by #2268
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix

Comments

@gilbert
Copy link
Contributor

gilbert commented Feb 28, 2018

I know stream is supposed to be m.prop's successor, but sometimes you just need a simple getter/setter function. What do you think of making a mithril/prop available to require on demand?

@barneycarroll
Copy link
Member

But Mithril streams are just an upgrade to props — they're fully backward compatible unless I'm mistaken. Maybe a blog post of the "you may not need (any of the stream methods)" variety would be the thing?

@gilbert
Copy link
Contributor Author

gilbert commented Mar 1, 2018

Streams are great, but it's still 100 lines vs a few. I'm probably being overly concerned about kb size :)

@orbitbot
Copy link
Member

orbitbot commented Mar 1, 2018

But Mithril streams are just an upgrade to props — they're fully backward compatible unless I'm mistaken.

👍

Maybe a blog post of the "you may not need (any of the stream methods)" variety would be the thing?

I'm not really sure that there's a particularly large group of actual target audience for this kind of thing...

@dead-claudia
Copy link
Member

dead-claudia commented Mar 3, 2018

@gilbert You might be interested in this. It's basically an m.prop with a little bit of sugar on top. There's only really two differences between it and the 0.2.x equivalent:

  1. You can observe changes now. That was the main complaint people had with the previous API.
  2. It doesn't implicitly wrap thenables now. Yes, it means it takes an extra line now, but it's less magical.

Edit:

Here's what I would actually add to core (one of two variants, each with their min+gzip added size):
// `m.prop`-like, adds 19 bytes min+gzip
m.prop = function (store, onchange) {
    return function (value) {
        if (!arguments.length) return store
        var old = store, func = onchange
        store = value
        if (typeof func === "function") {
            onchange = null
            func(store, old)
            onchange = func
        }
        return value
    }
}

// Separate getter/setter-like, adds 18 bytes min+gzip
m.prop = function (store, onchange) {
    return {
        get: function () { return store },
        set: function (value) {
            var old = store, func = onchange
            store = value
            if (typeof func === "function") {
                onchange = null
                func(store, old)
                onchange = func
            }
            return value
        }
    }
}

@osban
Copy link
Contributor

osban commented Mar 14, 2018

On one hand I'm not that concerned about the few kb extra, but on the other hand so far I've only used streams in an m.prop kind of way, as a getter-setter. I guess that means "I'm with you fellas" ;)

@JAForbes
Copy link
Collaborator

I'd be for 0.2x m.prop without the promise stuff, but I don't think m.prop should have change handlers. I think it encourages "doing instead of being" as Conal Elliot puts it. If we want to respond to changes in a prop, there's a really good way to do that and we already have it in mithril/stream.

So 👍 to m.prop, but if we want reactivity I think mithril should just make stream part of the main release and accept the extra 1.5kb because that 1.5kb is so so so useful.

If we include onchange I think we'll divert developers from stream because onchange is more familiar. I think many of us have tried the onchange thing in the past before we had streams and felt how awkward it is in practice. And the net result will be confusion for new comers, it makes it that much harder to define and explain m.prop's purpose and distinction from m.stream.

@isiahmeadows I like having separate get/set methods, providing they are bound. If we made that change to m.prop and m.stream I'd get behind that, I've hit issues with the existing api when composing many times.

@gilbert
Copy link
Contributor Author

gilbert commented Mar 14, 2018

I like these suggestions. Keep m.prop plain. Separate get/set functions seem to add good flexibility for not much overhead.

@dead-claudia
Copy link
Member

So removing the onchange handler, here's how it changes min+gzip:

  • Single function: 8 bytes dropped*
function prop(store) {
	return function (value) {
		if (arguments.length) store = value
		return store
	}
}
  • Getter/setter pair: 3 bytes dropped*
function prop(store) {
	return {
		get: function () {
			return store
		},
		set: function (value) {
			return store = value
		}
	}
}

* Yes, seriously. Both of these variants shaved the bundle size somehow...

@tivac
Copy link
Contributor

tivac commented Mar 15, 2018

@isiahmeadows are those drops in the gzipped size? That'd explain why adding bytes can sometimes drop the overall size.

@dead-claudia
Copy link
Member

dead-claudia commented Mar 15, 2018 via email

@tivac
Copy link
Contributor

tivac commented Mar 15, 2018

Since m.prop was such an apparently-popular part of 0.2x I'm fine with it returning as a simple getter/setter variant w/o the weird promise interactions.

I am very surprised by the number of people advocating for it, I hardly ever used it 😅

@dead-claudia dead-claudia added the Type: Enhancement For any feature request or suggestion that isn't a bug fix label Oct 28, 2018
@dead-claudia
Copy link
Member

I'll write a quick PR with this here soon-ish with the separate get/set pair. It should be pretty trivial.

dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Oct 29, 2018
@dead-claudia dead-claudia mentioned this issue Oct 29, 2018
11 tasks
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Oct 29, 2018
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Oct 29, 2018
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Oct 29, 2018
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Oct 29, 2018
dead-claudia added a commit to dead-claudia/mithril.js that referenced this issue Oct 29, 2018
@dead-claudia
Copy link
Member

Okay, #2268 is up.

dead-claudia added a commit that referenced this issue Nov 7, 2018
@dead-claudia dead-claudia moved this to Completed/Declined in Feature requests/Suggestions Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
Status: Completed/Declined
Development

Successfully merging a pull request may close this issue.

7 participants