940550 - Implement FRP window signals

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Irakli in our meeting today you said this was implemented already, such that 

```js
windows.on("data", function() {
    
})
```

from https://etherpad.mozilla.org/3INuTUVwJq

would track all windows, including those that are already open, and those that will open in the future.  I can't find the code that you are referring to.

Can you please either point me to it or resolve this?

Also I think the signature above is too similar (it's exactly the same afaict) to regular events from browser/events and window/events which are all for future windows.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Erik I think I was talking about two different things and I clearly
made this confusing. On one hand I was referring to this comment:

https://bugzilla.mozilla.org/show_bug.cgi?id=833011#c4

On the other I was trying to explain how we don't need window tracker for
our code itself as we should be hosting all event handlers at the top level.

I'm not going to repeat that here as we talked about this during MenuItem
related meeting & I also followed up with more clarifications here:

https://bugzilla.mozilla.org/show_bug.cgi?id=833011#c13



So let's ignore that snippet. Let's discuss WindowTracker replacement in Bug 833011.
We still need some sugar for handling and sending events though and this is right
bug for it. I'll try to get some examples shortly and ask for feedback. After that
we can finalize agreed API in JEP.

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1315

Pointer to Github pull-request

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8341373 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1315

This is not complete yet & I'm going to be adding more stuff but still maybe worth taking a look at.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I have attache pull request to signal JEP to a Bug 940510. Also posting link here:
https://github.com/mozilla/addon-sdk/pull/1316

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8341373 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1315

Looks great, I think we talked about using a Reactor thing in another bug though which I still like.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Implementation

I think it may require some updating which I'll try to get to soon. Still may be worth starting a review since it's a lot of code ;)

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Am I reviewing elmjs? If this is a separate library, we shouldn't be running tests for it inside the SDK. If it is not a separate library, it should probably be called `lib/frp` or something, not elm-specific. Does this review include reviewing elmjs or just the SDK components that consume it?

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It includes reviewing all of it. There is no such thing as elmjs, at least not right now. The reason it's not in `lib/frp` is the same as in other cases where we've added `lib/non-sdk` stuff, which is it does not really part of SDK although we may use it from SDK.

That being said I'm not totally against rearranging layout, but maybe we can discuss this separate from the actual code.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Just noticed that both 'method' and 'diffpatcher' also run tests in the SDK -- generally I don't like running tests of dependencies, because they should already be tested, unless if these are thought of as not dependencies but just different parts of the SDK? If that's the case, if I wanted to update method/diffpatcher/elmjs, would I have to send a PR to gozala/Method? I guess I don't understand how we should use these separate libraries.

Either way, if this library is to be maintained and live inside the SDK, naming it 'elmjs' doesn't provide usefulness to others who don't already know elm. Are we testing these inside the SDK? Then we should send PRs to the SDK for these libraries -- if not, the tests should run inside of their own repo, like gozala/*.

I guess it's not a matter of just rearranging the layout, it's a matter of how to handle "external" dependencies, since these aren't really other libraries, just libraries that you made specifically for this. Are we required to maintain them as well? I worry that elmjs tries to do many parts of FRP/elm, but not necessarily only the parts that we need currently, and that's needlessly expanding the parts of the SDK we do need to maintain.

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comments above ^

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9)
> Just noticed that both 'method' and 'diffpatcher' also run tests in the SDK
> -- generally I don't like running tests of dependencies, because they should
> already be tested, unless if these are thought of as not dependencies but
> just different parts of the SDK? 

I think of them as forks of the libraries that are sync with upstream in a frequency
that makes most sense for SDK project, in some cases it also may mean no syncing at
all and we evolve fork as we see it fit.

As of testing, we should test all the dependencies we use, unless library has automated
tests for the SDK environment, which I'm afraid is not the case for any library yet. Even
if library is well maintained for node browser and maybe elsewhere that does not give us
any guarantees it's going to work fine on SDK. As a matter of fact some of the bugs in
method library were occurring only in SDK.

>
> If that's the case, if I wanted to update
> method/diffpatcher/elmjs, would I have to send a PR to gozala/Method? I
> guess I don't understand how we should use these separate libraries.
>

Changes to any dependency shipped with SDK go through the same process as are changes
to the sdk core itself. It's up to you weather you wanna propose changes made in the fork
to an original library forked. In case of method library I just occasionally sync changes as
there has not being any change in SDK that would only make sense for SDK.



> 
> Either way, if this library is to be maintained and live inside the SDK,
> naming it 'elmjs' doesn't provide usefulness to others who don't already
> know elm.
>

It makes it a lot easier to pull down changes made in the library into a fork shipped
with SDK. And while I agree it does not provide usefulness to those who don't know elm,
but it does provide usefulness to those who do or would like to know.

>
> Are we testing these inside the SDK? Then we should send PRs to
> the SDK for these libraries -- if not, the tests should run inside of their
> own repo, like gozala/*.
>

Here you are not being very clear, so I'll try to answer to what I'm guessing you meant to say.
Yes we should test this inside SDK, since even those libraries have automated testing for node
and browsers that does mean there may not be issues specific to SDK that will cause misbehavior.
Luckily running their tests in SDK has being pretty trivial so I don't see reason why we should
not.

I do not agree that we should take overhead of sending changes to all third party libraries that
we decide to fork. Ideally we should not be making those forks, if we do, likely we picked a wrong
library to fork. And in case we do have to make changes to those forks, it still does not makes
much sense to waste resources to push those changes to repo we forked from. That being said it would
make sense to have a volunteer who would attempt to push SDK changes to third party libraries we fork.


> 
> I guess it's not a matter of just rearranging the layout, it's a matter of
> how to handle "external" dependencies, 

I think my comments above probably made my view on third party dependencies clear,
although if there are constraints with this approach let me know of them & if they
are serious enough we could even revisit currently used approach. But since interaction
with third party libs has being quite painless till now, I'd rather leave things as
is.

> since these aren't really other libraries, just libraries that you made specifically for this.

That's is only partially so. Method library has existed in node for quite some time before I recognized
benefits it brought to an SDK. After I have discussed pros and cons with a team and it was decided
to give it a try. It turned out that it solve one of the reoccuring problem we had, so it sticked.
In case of diffpatcher it's just I run into a same problem (which I solved years ago) so many times that I discussed with a reviewer and we pulled it in. In case of elmjs it's somewhat similar, we have being doing
some FRP stuff with event emitters and everyone on a team pointed out we should just use real signals as
otherwise it's just too confusing. As I have put ton of effort in elmjs prior to that it made sense to propose pulling it.

> Are we required to maintain them as well?

No. Third party libraries that we bring into SDK are forks (that's why they are not git submodules)
that we can modify as it makes most sense to SDK. That being said often times it makes sense to wrap
third party code to implement SDK specific changes rather than modifying forks as it makes a lot easier
to pull bugfixes in original libraries back to SDK. 


> I worry that elmjs tries to do many parts of FRP/elm, but not necessarily only the parts that we
> need currently, and that's needlessly expanding the parts of the SDK we do need to maintain.

I think you worry to much :) As of elmjs, SDK will definitely will need just a core that implements FRP signal, for example elmjs's graphics layer won't be applicable, at least not in foreseeable future. So SDK will keep only stuff it needs and won't pull other parts. It's also easy enough to talk to elmjs maintainer (me :) to make sure that graphics layer and core signal library will remain different libraries (which is a goal already).



Now here is a subjective view on this subject. It would be nice to have a volunteer who will try
to push changes made in SDK forks up to original repos. In earlier cases I was such volunteer, I was just pulling changes made to method in SDK to include them in a next release of method lib. I plan to do the same with elmjs. I have also being pulling changes from individual libraries back to sdk to keep them reasonably synced. Doing such maintenance was really easy given that structure of the repos has being preserved. Now if we decide to scatter those libraries by inlineing them into core SDK I am afraid I won't volunteer syncing changes as a cost of maintenance work will exceed the one I can spare for those libraries.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8425068 [details] [review]
Implementation

Review was address, but I don't believe this is being merged anymore?

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Irakli, I'm not sure if you want to work on this, if not could you please unassign yourself.

By the way, what is the state of this work?

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Erik implementation was there for a while which I think was waiting on our discussions about this to be complete so if we do want to land this it's about reviewing it at this point. That being said I'm not sure if it is still worth pursuing large changes like this.

What do you think ? If you have an interest in landing these changes I won't mind doing spending time in addressing review comments to get it there. I would also probably want to make some changes to the currently attached implementation if we do decide it's worth pursuing it.

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #14)
> Erik implementation was there for a while which I think was waiting on our
> discussions about this to be complete so if we do want to land this it's
> about reviewing it at this point. That being said I'm not sure if it is
> still worth pursuing large changes like this.
> 
> What do you think ? If you have an interest in landing these changes I won't
> mind doing spending time in addressing review comments to get it there. I
> would also probably want to make some changes to the currently attached
> implementation if we do decide it's worth pursuing it.

I remember trying to update this old branch and there were a few tricky things still not working.

Anyhow I do think landing smaller pieces is going to work out better, it will be much easier to review, and probably easier to land.

Note that Mossop has been working on a windows/tabs rewrite too.

Comment 16

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562