Candidate fix for #595 #685

lukewestby · Aug 8, 2016

This PR demonstrates one possible way to deal with calls to send prior to incoming ports being ready ( #595 ). As @artisonian mentioned, send can be called before onEffects in the port effect manager is called, so there are no Subs around to handle the send. This fix demonstrates queuing things that are sent until onEffects has been called for the first time, and then pushing all enqueued values through during that first call.

By @yjkogan and myself.

We demonstrated that this fixes the issue in https://github.com/lukewestby/elm-port-bootup-sscce

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot · Aug 8, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

knewter · Aug 9, 2016

knewter reviewed Aug 9, 2016

View changes

knewter · Aug 9, 2016

src/Native/Platform.js

+		if (!onEffectsCalled) {
+			onEffectsCalled = true;
+			enqueuedBeforeSetup.forEach(internalSend);
+			enqueuedBeforeSetup = null;


should this be turned into an empty array again? Feels weird changing its type.

I don't think it'll really matter.

Switch to use the style in all the rest of the JS. Notice that { is always brought down a line. Notice that } else { is always split onto three lines. Also, can we switch forEach out for a for loop? I'm not sure how old forEach is and I avoid things like this in the JS I write or generate as much as possible.

evancz · Aug 9, 2016

Switch to use the style in all the rest of the JS. Notice that { is always brought down a line. Notice that } else { is always split onto three lines. Also, can we switch forEach out for a for loop? I'm not sure how old forEach is and I avoid things like this in the JS I write or generate as much as possible.

There are two alternate approaches.

There is no such thing as onEffectsCalled, only queue = []. If it is instanceof Array you push values on it. If you send all the things, you switch queue to be null or undefined or something. This way there's no potential for synchronization issues between two different variables.
Rather than having ifs, switch between two different send functions. The first one queues things. When it's time to switch, you send all the things and switch to the real send function.

Why is your approach better than these?

evancz · Aug 9, 2016

There are two alternate approaches.

There is no such thing as onEffectsCalled, only queue = []. If it is instanceof Array you push values on it. If you send all the things, you switch queue to be null or undefined or something. This way there's no potential for synchronization issues between two different variables.
Rather than having ifs, switch between two different send functions. The first one queues things. When it's time to switch, you send all the things and switch to the real send function.

Why is your approach better than these?

Of the option we posted and the two alternatives I think alternative 2 is the best approach. It has the same benefit as alternative 1 of limiting the potential for variables to become out of sync and it also allows for the onEffects and send implementations in use for the majority of the application's lifetime to not need to check some condition that will never be relevant again.

I'll for sure change the forEach to a loop - forEach isn't present in IE8.

lukewestby · Aug 9, 2016

Of the option we posted and the two alternatives I think alternative 2 is the best approach. It has the same benefit as alternative 1 of limiting the potential for variables to become out of sync and it also allows for the onEffects and send implementations in use for the majority of the application's lifetime to not need to check some condition that will never be relevant again.

I'll for sure change the forEach to a loop - forEach isn't present in IE8.

Cool. Also, thanks for figuring this out and working on the PR!

evancz · Aug 9, 2016

Cool. Also, thanks for figuring this out and working on the PR!

No problem! It seems to be affecting a number of people and I got to hang out and code with @yjkogan for a couple hours 😄 win/win/win

Updates coming shortly

lukewestby · Aug 9, 2016

No problem! It seems to be affecting a number of people and I got to hang out and code with @yjkogan for a couple hours 😄 win/win/win

Updates coming shortly

Looks awesome!

yjkogan · Aug 10, 2016

Looks awesome!

Cool :) The solution feels a little tricky, but I think that's inherent to the problem. I.e. This seems like the best way to do a hard thing. Point is, thanks @lukewestby and @yjkogan for sorting this one out! :D (Also hi!)

Do you think it makes sense to do 4.0.5 to get this out as quick as possible? I'll have to look back and see if there are any other changes on master, but that seems like the way to go to me.

evancz · Aug 10, 2016

Cool :) The solution feels a little tricky, but I think that's inherent to the problem. I.e. This seems like the best way to do a hard thing. Point is, thanks @lukewestby and @yjkogan for sorting this one out! :D (Also hi!)

Do you think it makes sense to do 4.0.5 to get this out as quick as possible? I'll have to look back and see if there are any other changes on master, but that seems like the way to go to me.

I'll defer to @yjkogan, @knewter and anyone else currently experiencing this in production to make a recommendation - I don't have any applications in this situation myself so I don't know how urgent it is.

Thanks!

lukewestby · Aug 10, 2016

I'll defer to @yjkogan, @knewter and anyone else currently experiencing this in production to make a recommendation - I don't have any applications in this situation myself so I don't know how urgent it is.

Thanks!

It's not urgent at all because you can always do a setTimeout(fn(){}, 0), it's just nice to keep new people from seeing the confusing result.

knewter · Aug 10, 2016

It's not urgent at all because you can always do a setTimeout(fn(){}, 0), it's just nice to keep new people from seeing the confusing result.

Eh, I just looked through the commits on master, and this is the only thing since 4.0.4 so I decided to just publish it. If you install things now, it should be 4.0.5 and this should be fixed.

Talk about it here if there are further issues. Otherwise, great work! :)

evancz · Aug 10, 2016

Eh, I just looked through the commits on master, and this is the only thing since 4.0.4 so I decided to just publish it. If you install things now, it should be 4.0.5 and this should be fixed.

Talk about it here if there are further issues. Otherwise, great work! :)

Woohoo! Late to the party but same as @knewter. It was easy to work-around once we found the issue but I think this makes the logic simpler (and lets our site load/paint faster!)

yjkogan · Aug 11, 2016

Woohoo! Late to the party but same as @knewter. It was easy to work-around once we found the issue but I think this makes the logic simpler (and lets our site load/paint faster!)

Thank you all for taking time to work on this issue!

bipvanwinkle · Aug 12, 2016

Thank you all for taking time to work on this issue!

add changes to enqueue sends to incoming ports before onEffects is ac…
…tually called

41c7bee

lukewestby referenced this pull request Aug 9, 2016
Closed
Ports do not register immediate `send` #595

Less coloquial function name

9b1c8e0

Luke Westby added some commits Aug 9, 2016

instead of checking conditions swap implementations of onEffects and …
…send

4be3245

missing semi

85f9c10

evancz merged commit efe096f into elm:master Aug 10, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

elm/core

Candidate fix for #595 #685

lukewestby commented Aug 8, 2016

This comment has been minimized.

process-bot commented Aug 8, 2016

lukewestby referenced this pull request Aug 9, 2016

Ports do not register immediate `send` #595

knewter reviewed Aug 9, 2016

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

evancz commented Aug 9, 2016

This comment has been minimized.

evancz commented Aug 9, 2016

This comment has been minimized.

lukewestby commented Aug 9, 2016 •

edited

Edited 1 time

lukewestby edited Aug 9, 2016 (most recent)

This comment has been minimized.

evancz commented Aug 9, 2016

This comment has been minimized.

lukewestby commented Aug 9, 2016 •

edited

Edited 1 time

lukewestby edited Aug 9, 2016 (most recent)

Luke Westby added some commits Aug 9, 2016

This comment has been minimized.

yjkogan commented Aug 10, 2016

This comment has been minimized.

evancz commented Aug 10, 2016

evancz merged commit `efe096f` into elm:master Aug 10, 2016
1 check passed

1 check passed

This comment has been minimized.

lukewestby commented Aug 10, 2016

This comment has been minimized.

knewter commented Aug 10, 2016

This comment has been minimized.

evancz commented Aug 10, 2016

This comment has been minimized.

yjkogan commented Aug 11, 2016

This comment has been minimized.

bipvanwinkle commented Aug 12, 2016

elm/core

Join GitHub today

Candidate fix for #595 #685

Conversation

lukewestby commented Aug 8, 2016

This comment has been minimized.

process-bot commented Aug 8, 2016

lukewestby referenced this pull request Aug 9, 2016

knewter reviewed Aug 9, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

evancz commented Aug 9, 2016

This comment has been minimized.

evancz commented Aug 9, 2016

This comment has been minimized.

lukewestby commented Aug 9, 2016 • edited Edited 1 time lukewestby edited Aug 9, 2016 (most recent)

This comment has been minimized.

evancz commented Aug 9, 2016

This comment has been minimized.

lukewestby commented Aug 9, 2016 • edited Edited 1 time lukewestby edited Aug 9, 2016 (most recent)

Luke Westby added some commits Aug 9, 2016

This comment has been minimized.

yjkogan commented Aug 10, 2016

This comment has been minimized.

evancz commented Aug 10, 2016

Hide details View details evancz merged commit efe096f into elm:master Aug 10, 2016 1 check passed

1 check passed

This comment has been minimized.

lukewestby commented Aug 10, 2016

This comment has been minimized.

knewter commented Aug 10, 2016

This comment has been minimized.

evancz commented Aug 10, 2016

This comment has been minimized.

yjkogan commented Aug 11, 2016

This comment has been minimized.

bipvanwinkle commented Aug 12, 2016

knewter reviewed Aug 9, 2016

View changes

lukewestby commented Aug 9, 2016 •

edited

Edited 1 time

lukewestby edited Aug 9, 2016 (most recent)

lukewestby commented Aug 9, 2016 •

edited

Edited 1 time

lukewestby edited Aug 9, 2016 (most recent)

evancz merged commit `efe096f` into elm:master Aug 10, 2016
1 check passed