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

Always call registry's trigger method from withRegistration #287

Closed
wants to merge 1 commit into from

Conversation

tgvashworth
Copy link
Contributor

This is to make it possible to add a runtime hook to trigger, even when the app is running out of debug mode (as TweetDeck does).

🚀

This is to make it possible to add a runtime hook to trigger, even
when the app is running out of debug mode (as TweetDeck does).
@angus-c
Copy link
Contributor

angus-c commented Jul 10, 2014

@sayrer and I put it behind the debug guard because we were concerned about perf. That's two extra function calls for every trigger, and there can be multiple triggers per click

@tgvashworth
Copy link
Contributor Author

If I benchmarked the difference, how much would be unacceptable?

@angus-c
Copy link
Contributor

angus-c commented Jul 10, 2014

Almost impossible to answer. What's the use case you need it for?

@tgvashworth
Copy link
Contributor Author

Am working on Flight tooling, hooking into trigger to show what's being triggered where amongst other things. Afaict, really the only hookable bit of Flight is the registry (exposed in standalone, mostly requireable in AMD), and the one method that isn't always exposed is trigger.

I would say it's fine to keep behind debug mode, but TweetDeck can't run in debug mode because we're passing non-seriablizable data around, and we aren't the only ones – the Airbnb search page does it too, and most likely others.

Unless there's another way to advice every component's trigger, added at runtime after the flight app may have loaded, the utility of & scope for the tools I'm building will be severely limited :/

My view is that two, essentially noop, function calls per trigger is fine.

@angus-c
Copy link
Contributor

angus-c commented Jul 10, 2014

idea from @kloots: do a this.after('trigger', function() {//...});

@tgvashworth
Copy link
Contributor Author

What's this in that example?

@angus-c
Copy link
Contributor

angus-c commented Jul 10, 2014

your component (just the regular advice pattern)

@tgvashworth
Copy link
Contributor Author

There isn't a component here – it's an external tool that hooks into Flight. That's why I need it on the registry – it's the only bit has externally exposed methods that are called with a component's context (from withRegistration).

Fwiw, I am actually using advice on trigger, but it's the registry's trigger – and that's why I need it to not be behind the debug flag.

@kloots
Copy link

kloots commented Jul 10, 2014

If you're already using advice to go around trigger(), could you not just have your wrapper for trigger() fire a custom event on the document that could then be listened to by any third party?

@tgvashworth
Copy link
Contributor Author

I'm not already using advice to go around trigger – I'm only able to do that when it's exposed and not behind the flag. This PR just makes it possible when the flag is off.

@sayrer
Copy link
Collaborator

sayrer commented Jul 11, 2014

I'd like to know why you're passing non-serializable data around before we start hacking things up to work around it.

@tgvashworth
Copy link
Contributor Author

@sayrer I don't know why we are, but we are.

To summarise again: I want a way to hook into every trigger on all components, with arguments. This is possible with registry.trigger, but the call cannot be behind a flag for the tools to be useful to TweetDeck and, it turns out, Airbnb. I'm sure there are others.

If there's an alternate way to do this without a PR, I'm all ears.

@sayrer
Copy link
Collaborator

sayrer commented Jul 11, 2014

I want to know why Tweetdeck can't use debug mode... this is what it's for. Restating the thread as a demand is a bit facile.

@tgvashworth
Copy link
Contributor Author

We have a huge amount of non-Flight code and so in some cases it's simply impractical to use something serializable to get data in and out of that layer.

@tgvashworth
Copy link
Contributor Author

Closing as we agreed offline to add the necessary data to the DEBUG global.

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.

4 participants