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

[FEATURE ember-metal-stream] #9693

Merged
merged 1 commit into from
Jan 6, 2015
Merged

[FEATURE ember-metal-stream] #9693

merged 1 commit into from
Jan 6, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Nov 24, 2014

Expose the base internal stream implementation as Ember.Stream.

@stefanpenner
Copy link
Member

@Private, or _Stream for now

@jmurphyau
Copy link
Contributor

Are Stream's ever going to be exposed?

I'm just wondering if the way I've implemented a 'lookup' helper should/will change. Since its supposed to return a stream that changes independent of either the 'obj' (params[0]) or 'key' (params[1]) streams I couldn't see how it could I could implement this as a regular bound helper and/or without Streams

export function lookupHelper(params, hash, options, env) {
    var obj = params[0];
    var key = params[1];

    if (isStream(obj) && isStream(key)) {
        var accessKey = "lookup:$%@-$%@".fmt(Ember.guidFor(obj),Ember.guidFor(key));

        //can't contain '.' characters otherwise it will call child properties
        accessKey = accessKey.replace(/\./,'-');

        var stream = this.getStream(accessKey);

        //make sure it's not setup more than once
        if (!stream._isLookup) {

            stream._objStream = obj;
            stream._keyStream = key;

            stream._setLookupSource = function() {
                // sets the source object of this stream. any changes to the value 
                // of the source stream will automatically trigger this stream to refresh

                var sourceStream,
                        keyVal = read(this._keyStream);

                if (!Ember.isNone(keyVal)) {
                    sourceStream = this._objStream.get(keyVal)
                } else {
                    sourceStream = null;
                }

                this.setSource(sourceStream);
            };

            // when the value for 'key' changes, call 'stream._setLookupSource'
            // which will set the 'source' to a new stream object. 
            subscribe(stream._keyStream, stream._setLookupSource, stream);

            stream._setLookupSource(); 

            stream._isLookup = true;   
        }

        return stream;
    }
}

@rwjblue
Copy link
Member Author

rwjblue commented Jan 4, 2015

Rebased, and updated to ensure everything exposed is properly marked as private.

@rwjblue rwjblue force-pushed the expose-stream branch 2 times, most recently from 4d89d83 to 61747d3 Compare January 4, 2015 23:34
@mixonic
Copy link
Member

mixonic commented Jan 5, 2015

@mmun this is for you to merge imo. It is behind a FF and private, low risk.

@jmurphyau we definitely want to expose them- but we're also wary of formalizing an API before it settles. If you need to reach for streams in your application code, it probably points to a missing API at a higher level.

@mmun
Copy link
Member

mmun commented Jan 6, 2015

@jmurphyau This seems like a good ember add-on: ember-cli-lookup-helper. It should probably know how to deal with streams/non-streams for both object and key (so 4 combinations). Our current implementation only deals with static keys.

EDIT: Actually may we can just add this to KeyStream. There's basically no cost to it.

@rwjblue
Copy link
Member Author

rwjblue commented Jan 6, 2015

Rebased.

@mmun
Copy link
Member

mmun commented Jan 6, 2015

@rwjblue I don't see the point of exposing them privately. It's not much better then Ember.__loader.require. Let's make em public!

@rwjblue
Copy link
Member Author

rwjblue commented Jan 6, 2015

@mmun - So make the util methods @public, but leave Ember._Stream as private? Or make 'em all public?

@mmun
Copy link
Member

mmun commented Jan 6, 2015

@rwjblue All public, I think!

I think we should jam everything into Ember.stream.*.

Ember.stream.read
Ember.stream.Stream
Ember.stream.KeyStream
Ember.stream.conditional
...

Expose the base internal stream implementation as `Ember.Streams.Stream`, and
the group of utility functions as `Ember.Streams.utils`.
@rwjblue
Copy link
Member Author

rwjblue commented Jan 6, 2015

@mmun - Updated.

mmun added a commit that referenced this pull request Jan 6, 2015
@mmun mmun merged commit 4a1010a into emberjs:master Jan 6, 2015
@mmun mmun deleted the expose-stream branch January 6, 2015 02:44
@mmun
Copy link
Member

mmun commented Jan 6, 2015

Excellenté

@@ -327,6 +341,23 @@ Ember.isPresent = isPresent;

Ember.merge = merge;

if (Ember.FEATURES.isEnabled('ember-metal-stream')) {
Ember.stream = {
Stream: Stream,
Copy link
Member

Choose a reason for hiding this comment

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

Ember.stream.Stream seems weird

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. This is my "fault". I wanted to put everything in Ember.stream._ because I expect it will become import _ from "ember-streams".

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.

6 participants