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

added postInitialize #176

Closed
wants to merge 2 commits into from
Closed

added postInitialize #176

wants to merge 2 commits into from

Conversation

bhamblok
Copy link

Now you can use a "postInitialize"-function which will be triggered just AFTER all "initialize"-functions from parent -states and -collections have been triggerd.
This is necessary when you need to access some (non-binding) properties from a parent-State-object which needs to be altered first in it's own initialize function.

@latentflip
Copy link
Contributor

I'm still trying to work out what's going on here, but it looks like this PR also includes the changes from #175, so we'll want to wait for a decision on that before we merge this, or redo this PR without those changes.

@bhamblok
Copy link
Author

Yes, sorry, I continued working from my previous branch... Shall I revert this into 2 different PR's? Waiting for a decision on the first PR is also fine for me.
Thanx

@bhamblok
Copy link
Author

Hi @latentflip, If you need more information, I'd love to talk about it. This is my first real contribution... So I'm just getting acquainted using Pull Request on Github.

@pgilad
Copy link
Member

pgilad commented Jun 18, 2015

I think you should create a new branch and add just the changes relevant for postInitialize to it. Then force push it to this branch to replace the previous commits.

@bhamblok
Copy link
Author

Hi @latentflip and @pgilad. I created a new branch to be able to replace the previous commits. I also updated the readme-file so it is more clear why I would like to have this functionality added to ampersand. Feel free to ask more information.

Thanks

@pgilad
Copy link
Member

pgilad commented Jun 19, 2015

Hey I'd actually like to raise the debate on whether view lifecycles should be done via callback functions or events.

Events are great in the form that any module can subscribe/unsubscribe to/from them. Also they don't force you to use a specific function that is coupled to the view instance. For example you can assign a helper function to handle the lifecycle event (say analytics tracking).

Most if not all amp modules have events mixed in to them, so it doesn't require adding another module. The only downside to events is that you are forced to take 2 steps in order to realize the lifecycle: 1. Subscribe to the event 2. Set up a handle/callback.

IMO events are preferable. Consider these 2 examples:

var UsingFunctions = State.extend({
    initialize() {
        console.log('init');
    },
    postInitialize() {
        console.log('realized');
    }
});

var UsingEvents = State.extend({
    initialize() {
        console.log('init');
        this.listenTo(this, 'post:initialize', this.postInitialize);
    },
    postInitialize() {
        console.log('realized');
    }
});

In the first example using functions - it seems like the postInitialize is being called via magic. I'm not sure how is it called as it relies purely on the setup of the ampersand-state module.

In the 2nd example using events - it requires an extra step, but it's far more explicit. I know why the postInitialize function is being triggered. Also, as an added bonus - I can add additional handlers, use a helper function (and not an instance function), and most importantly - I can unsubscribe at will.

I vote for using events as lifecycle hooks.

@pgilad
Copy link
Member

pgilad commented Jun 20, 2015

Actually I did some testing, and the problem with events, at least with the current implementation of trigger is that you are unable to create non-bubbling events. That means if you have a model inside a collection triggering post:initialize it will bubble to the collection - and if it has a listener - will trigger a false positive.

So either we create an alternative trigger method (i.e triggerDull), or using functions will be better.

@bhamblok
Copy link
Author

Thanks for your research and testing. You're right that using events is a more explicit way, but the bubbling-problem you describe indeed makes this impossible.
In our opinion an alternative trigger method is even more magic as using functions. Knowing when to use "trigger" vs "triggerDull" would only complicate things while functions are more basic javascript. On the other hand, an alternative trigger method might open up more use-cases.

@bhamblok bhamblok closed this May 25, 2016
@bhamblok bhamblok deleted the postInit branch May 25, 2016 13:51
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.

3 participants