Improve Reactivity #230

calebporzio · 2020-03-05T05:11:22Z

This PR swaps the hand-rolled Proxy-based reactivity core in Alpine for Saleforce's "observable-membrane" library: https://github.com/salesforce/observable-membrane

Reasoning: Solving the current Alpine reactivity issues feels like playing bug whack-a-mole for the project. With every issue, we have to dig deeper into JS Proxies and add extra conditionals to the Proxy core.

By using this off-the-shelf lib, we are accepting the cost of an extra ~1kb min/gzipped added to Alpine very small size, AND adding our first real JS dependancy, but in return, we are getting reliability and peace of mind.

There are other packages out there for this, specifically: https://github.com/nx-js/observer-util

But that package hasn't been touched since May of 2019, and this one is maintained by Salesforce which is a giant company that presumably uses this same lib in their "Lightening Web Components" framework core. I'm much more confident that it'll stick around and get maintained.

This concept of a "Proxy-based Object Membrane" is the exact concept behind the new VueJS 3 reactivity core. It looks like they hand-rolled their core, but in the README, they said it was heavily inspired by these packages.

I think this is a step in the right direction for Alpine.

…ctivity

calebporzio · 2020-03-05T05:13:05Z

@SimoTod - what do you think?

I'm pretty sold on this, it took me 2 minutes to pull in and get working, it fixed the bugs I was aware of, including the issue your latest PR solved.

I'd love it if you could pull this down and test for yourself.

Also, do you think this warrants tagging 3.0? I'd rather make this a minor release, but I am fearful. Thoughts?

SimoTod · 2020-03-05T07:01:28Z

If we can use an existing solid implementation, I'm down with that.
Probably that additional 1k covers a lot of edge cases which we haven't considered yet but will come up as a bug as some point.
I'll have a play later today and I'll let you know my thoughts but it should be safe to swap and since it shouldn't introduce breaking changes, I don't think it requires a major release.

HugoDF · 2020-03-05T08:00:16Z

@calebporzio I don't think this is a breaking change, 1kb is the same amount that other minor feature PRs have added (should be a minor bump) plus the reactivity system isn't exposed to the outside of AlpineJS right?

The other question is IE11 support, does this library still work in IE11?

HugoDF · 2020-03-05T08:01:19Z

src/component.js

@@ -65,28 +68,25 @@ export default class Component {
    }

    getUnobservedData() {
-        let rawData = {}
+        let unwrapedData = this.membrane.unwrapProxy(this.$data)


typo across this function

Suggested change

let unwrapedData = this.membrane.unwrapProxy(this.$data)

let unwrappedData = this.membrane.unwrapProxy(this.$data)

Good catch, thanks!

ruiposse · 2020-03-05T11:04:15Z

Playing devil's advocate here: isn't this against the "Alpine" philosophy ?

I would actually expect to have an "alpinejs-reactivity-core" module that other frameworks could say "let's just use this small module to solve reactivity reliably for us" the same the dependency you're introducing does (but in an "alpinejs way").

Maybe this is not the popular comment, but I'd love to hear your thoughts about it.

ryangjchandler · 2020-03-05T12:31:43Z

Playing devil's advocate here: isn't this against the "Alpine" philosophy ?

I would actually expect to have an "alpinejs-reactivity-core" module that other frameworks could say "let's just use this small module to solve reactivity reliably for us" the same the dependency you're introducing does (but in an "alpinejs way").

Maybe this is not the popular comment, but I'd love to hear your thoughts about it.

I'd semi-agree with you. I don't think there's anything wrong with using a 3rd party lib if it covers lots of edge cases and is more solid as @SimoTod mentioned, but I'd like the idea of having a separate @alpinejs/reactivity-core package too. At that point though, it seems as though you'd be turning Alpine into a mono-repo, which I'm not sure we'd want.

Some benefits would involve making changes to the reactive core without needing a new release of Alpine, since the version constraints would always use the latest supported version of the core packages, unless the sub-packages are versioned inline with the parent Alpine repo, such as the illumiante/* and laravel/framework package.

ruiposse · 2020-03-05T14:07:04Z

Fair enough on the mono-repo argument.
But I'd still expect something called Alpine.js not to have real dependencies. It might be that with a philosophy like this you need to compromise on a lot of things - but that's exactly what Alpine Linux does.

calebporzio · 2020-03-05T14:34:20Z

Interesting thoughts. Thanks everyone for pitching in.

Good insight on this not being a breaking change. I won't tag it as such.

As far as straying from the "Alpine Philosophy", I don't think a more reliable, third-party reactivity core strays from that. If we moved to a v-dom core, I would agree, but this to me is just an implementation detail.

To your point though, I could see the argument for challenging the status quo and rolling our own, very small, "as-needed" reactivity core (like we were doing), but honestly, I'd rather spend our time making Alpine awesome than wrapping our heads around obscure Proxy workarounds.

HugoDF · 2020-03-05T14:39:39Z

rollup.config.js

        resolve(),
        filesize(),
-        terser({


Any reason you're dropping this?

The built alpine.js asset is now unminified

yeah, it only saves like .1KB

Seems not worth it to me. Unminified means way easier debugging.

But it's a good idea to leave it there for when the source code grows bigger. But we can add it later anyways. It's fine. ~~Although @calebporzio this will break the code for people who used the CDN links previously. Should be removed starting from the next major release.~~

Removing terser shouldn't remove code so not really a breaking change

Have you tried it with mangle: true, seems like that would save a bunch more size by renaming variables.

Hi @HugoDF. Thanks for your comment. For some reason, I thought we already had a minified version of the alpine in the "dist" folder. But we actually didn't.

This was the "dist" content before merging this pull request.
https://github.com/alpinejs/alpine/tree/a6496895b4ddea560244c43e4e83407861814a70/dist

JSDeliver supports auto minifying (by adding ".min" before the file extension) so it's not actually needed at all. Sorry for my misleading statement @calebporzio

iksaku · 2020-03-05T16:01:41Z

Regarding browser compatibility (@HugoDF), in the README.md file of observable-membrane, they specify the need of ECMAScript 6 Proxy.

This means no IE11 compatibility afaik.

More on this:

HugoDF · 2020-03-05T16:08:05Z

@iksaku I think the Proxy polyfill is included in the IE11 Alpine build

SimoTod · 2020-03-05T19:09:49Z

hi @calebporzio

I tried several examples and that library seems solid: All my old pens about proxy issues seem to work fine and another couple of things I tried locally where working as expected.

The only thing I noticed (It's not an existing feature, just a branch I was working on), is that object are not reactive if you change their state using functions.
For example a Date object has a bunch of setters, I was imagining someone trying to build a date picker so they probably would expect Alpine to react to those setters but I guess we need to draw a line somewhere.

This is the example using the new lib: https://codepen.io/SimoTod/pen/eYNGLwK
If you click + or -, you can see in console that the object changes but the span doesn't react unless you change another properties to trigger the whole refresh (I've added the refresh function)
This is the branch I was working on https://codepen.io/SimoTod/pen/jOPmoeN.

It doesn't fix #144 and #158 but those are related to the extraVars object, really.

I'm personally not against dependencies if they allow you to focus on other features.
Laravel uses some Symfony components but that doesn't make Laravel less awesome.
If you feel that this is the right direction, go for it. 👍

HugoDF

(one more review comment) it seems deepProxy is still being imported into component despite having been deleted 😄

https://github.com/alpinejs/alpine/pull/230/files#diff-d24fe73020f46dde060f58786e987b61R1

calebporzio · 2020-03-06T03:46:27Z

Thanks for all the input. The typo and unused import are fixed. Merging!

HugoDF · 2020-03-06T07:53:35Z

@calebporzio I think you just merged/released with unminified dist/alpine.js

philippbosch · 2020-03-06T08:37:12Z

@HugoDF see his comment here: #230 (comment)

HugoDF · 2020-03-06T08:48:37Z

@philippbosch thanks for pointing that out, I raised an issue/created a PR for IE11 build being broken

philippbosch · 2020-03-06T08:49:45Z

@HugoDF yeah, I saw that. I'm also relying on the IE11 build so thanks for that!!

Swap hand-rolled for salesforce's observable-membrane package for rea…

a649689

…ctivity

HugoDF reviewed Mar 5, 2020

View reviewed changes

Fix typo and unused import

6776574

calebporzio merged commit eafbc4b into master Mar 6, 2020

This was referenced Mar 6, 2020

Broken IE11 build + unminified base build #241

Closed

Fix IE11 build, reintroduce minification to regular build #242

Merged

HugoDF mentioned this pull request Mar 7, 2020

Not minified since 2.1 release #249

Closed

SimoTod mentioned this pull request Mar 10, 2020

Access nested items in x-for #158

Closed

calebporzio deleted the swap-reactivity-engine branch March 17, 2020 19:02

ryangjchandler mentioned this pull request Apr 15, 2020

Minify builds #381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Reactivity #230

Improve Reactivity #230

calebporzio commented Mar 5, 2020

calebporzio commented Mar 5, 2020

SimoTod commented Mar 5, 2020

HugoDF commented Mar 5, 2020 •

edited

Loading

HugoDF Mar 5, 2020

calebporzio Mar 5, 2020

ruiposse commented Mar 5, 2020

ryangjchandler commented Mar 5, 2020 •

edited

Loading

ruiposse commented Mar 5, 2020

calebporzio commented Mar 5, 2020

HugoDF Mar 5, 2020

calebporzio Mar 6, 2020

the94air Mar 6, 2020 •

edited

Loading

HugoDF Mar 6, 2020

the94air Mar 6, 2020

iksaku commented Mar 5, 2020

HugoDF commented Mar 5, 2020

SimoTod commented Mar 5, 2020 •

edited

Loading

HugoDF left a comment •

edited

Loading

calebporzio commented Mar 6, 2020

HugoDF commented Mar 6, 2020

philippbosch commented Mar 6, 2020

HugoDF commented Mar 6, 2020

philippbosch commented Mar 6, 2020

	let unwrapedData = this.membrane.unwrapProxy(this.$data)
	let unwrappedData = this.membrane.unwrapProxy(this.$data)

Improve Reactivity #230

Improve Reactivity #230

Conversation

calebporzio commented Mar 5, 2020

calebporzio commented Mar 5, 2020

SimoTod commented Mar 5, 2020

HugoDF commented Mar 5, 2020 • edited Loading

HugoDF Mar 5, 2020

Choose a reason for hiding this comment

calebporzio Mar 5, 2020

Choose a reason for hiding this comment

ruiposse commented Mar 5, 2020

ryangjchandler commented Mar 5, 2020 • edited Loading

ruiposse commented Mar 5, 2020

calebporzio commented Mar 5, 2020

HugoDF Mar 5, 2020

Choose a reason for hiding this comment

calebporzio Mar 6, 2020

Choose a reason for hiding this comment

the94air Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

HugoDF Mar 6, 2020

Choose a reason for hiding this comment

the94air Mar 6, 2020

Choose a reason for hiding this comment

iksaku commented Mar 5, 2020

HugoDF commented Mar 5, 2020

SimoTod commented Mar 5, 2020 • edited Loading

HugoDF left a comment • edited Loading

Choose a reason for hiding this comment

calebporzio commented Mar 6, 2020

HugoDF commented Mar 6, 2020

philippbosch commented Mar 6, 2020

HugoDF commented Mar 6, 2020

philippbosch commented Mar 6, 2020

HugoDF commented Mar 5, 2020 •

edited

Loading

ryangjchandler commented Mar 5, 2020 •

edited

Loading

the94air Mar 6, 2020 •

edited

Loading

SimoTod commented Mar 5, 2020 •

edited

Loading

HugoDF left a comment •

edited

Loading