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

move obs(...args) to obs.get(...args) #8

Open
ahdinosaur opened this issue Mar 23, 2017 · 6 comments
Open

move obs(...args) to obs.get(...args) #8

ahdinosaur opened this issue Mar 23, 2017 · 6 comments

Comments

@ahdinosaur
Copy link

when using observables as both functions and values, there's a slight problem with how the details of Function cause this hack when converting between values and functions. basically you can't have an observable with properties name or length, because those are part of the function.

what if instead, we changed the API so that obs(). became obs.get()? this would be enough to fix the overlap. then an "observable" becomes an object with a .get and .set function.

this would be a breaking change from observ. 😅

@mmckegg
Copy link
Owner

mmckegg commented Mar 23, 2017

Oh yeah. This old thing.

I suppose I'm more attached to the "Observable as a function" thing for sentimental reasons. It would be a real shame to see it go, but not being able to have "name" and "length" keys really does suck.

Keep in mind that this would mean you couldn't have a get key. But I suppose that's a lot less problematic than no name key.

Hmmmmmmmm. I'll have to think about this. The real question is, what are the disadvantages? Are there any?

@ahdinosaur
Copy link
Author

yeah, so far the only disadvantage i see is the additional code to call obs.get() instead of obs(), which breaks mutant from being compatible with the observ ecosystem. but maybe this is a feature too since it's more self-descriptive.

an alternative is we change the "Struct" format so it's like obs.get.name or something, i think.

@mmckegg
Copy link
Owner

mmckegg commented Mar 24, 2017

Another other interesting side effect:

We will no longer be assuming that functions are observables. Maybe check for presence of .get or a magic key?

This will allow the onevent style handlers to work as expected. resolve and isObservable will have to change (hopefully this will reduce the number of places that have to be changed elsewhere).

@ahdinosaur
Copy link
Author

mhm. i'm keen for using a magic key or global symbol like Symbol.for('mutant/isObservable').

ah yep, cool.

@mmckegg
Copy link
Owner

mmckegg commented Jul 6, 2017

@mmckegg
Copy link
Owner

mmckegg commented Aug 6, 2017

Thinking about this again. The biggest problem is this is a major breaking API change that would require refactoring a lot of projects.

But here's an idea: modify isObservable, resolve, and watch to check for the presence of the mutant/observable symbol (these are used internally for all checks). Expect the symbol to return a function. Run that function instead of obs(). We can do the same thing for set with a mutant/setter symbol. We can then apply these symbols to all existing mutant types. We don't need to change them away from being functions, and can leave the old .set in place. The API stays the same, but now mutant types do not have to be functions or implement specific functions.

This means that you can implement a custom mutant type that is not a function, but the old API is not broken in any way. It also means we can get the advantages of stricter isObservable checking on html-element. onevent will work as expected and no longer detect functions as obserables!

@ahdinosaur you could create a mutant-struct library that allows any key as a separate package and it would integrate in to the rest of the observables.

This also means that any observable library could potentially integrate with mutant if it mapped the symbols correctly 😄

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

No branches or pull requests

2 participants