GoogleChromeLabs / comlink

Comlink makes WebWorkers enjoyable.
Apache License 2.0
11.41k stars 390 forks source link

Communication between 4.3.0 and 4.3.1 fails #540

Open deasems opened 3 years ago

deasems commented 3 years ago

I'm using comlink with iframes and I'm seeing communication between frames fails when the parent app is running v4.3.1 and the child app is running 4.3.0. Specifically the promise returned from this line never seems to resolve:

// child app
wrap(windowEndpoint(window.parent, window, targetOrigin)).callParentMethod();

This is a major issue for us because the two different frames are separate apps maintained by different teams and have different release schedules so it impractical for us to coordinate a comlink upgrade. For now we are pinning our versions to 4.3.0

surma commented 3 years ago

yeah, this was diagnosed in #537

My bad for not realizing that this would actually be a breaking change.

How do you recommend this be handled? 4.3.1 is released now, is it better to roll back the change and release 4.3.2? Unpublish 4.3.1?

MrMadClown commented 3 years ago

Unpublishing 4.3.1 would fix the issues of those who use the latest cdn version, but would break any build which depends on 4.3.1 in the package.lock.json

I would probably publish a prior commit as 4.3.2 and move on with a new version 4.4. That way, cdn usage is fixed and builds don't break. EDIT: cdn usage isn't necessarily fixed since the example from the other issue used the latest version, it would also have been broken with a new mayor version.

I don't think there is a solution which works without anyone who depends on comlink taking action in up/downgrading

deasems commented 3 years ago

@surma I think @MrMadClown suggestion makes sense for a 4.3.x hotfix.

Going forward could this 4.3.1 change could be implemented in a non breaking manner? Just marking it as a new major version makes sense semantically but we'd be stuck with 4.x because we can't really coordinate two separate app release pipelines for a comlink upgrade and wouldn't want to risk downtime. So my ask would be to have some migration strategy to support upgrading comlink versions independently in the parent/child frames.

updatedData commented 3 years ago

So basically everyone that has different versions didn't test the application after they updated their libraries. The changes are internal, so there is no breaking change in my opinion. Actually it's fixing a bug, a bug that caused the applications to work in the first place.

When you go with bleeding edge this is what you get.

maksnester commented 3 years ago

Unpublish 4.3.1?

Definitely not, some people already upgraded to it and it works fine. Unpublishing will break those apps/libs.

I can think of only 2 possible options here:

  1. Do nothing. Make people sad but force them to update everything to the latest version.
  2. Publish 4.3.2 with revert to make those apps affected by 4.3.1 work again (once updated). Go for 5.0 with breaking changes. Yeah, API remained the same, but does it matter if apps are broken after the update?

I'm personally OK with the 1st option here since I already spent some time updating things 😄

guanzo commented 3 years ago

If you change the protocol then it's a breaking change, regardless if the public API is unchanged.

I remember when a minor version upgrade of Comlink broke my app (the shape of the error object was changed), so I pinned Comlink to its minor version. Now a patch version upgrade broke my app, so now I have to pin Comlink to a specific version.

I don't think Comlink makes it clear enough that you must use compatible versions of Comlink in all of your frames. Compatible here meaning, basically the same exact version. With the way comlink is versioned - protocol changes introduced in minor/patch versions - it's too easy to break production.

sidoruk-sv commented 2 years ago

Hi! Any progress on this?

benjamind commented 1 year ago

We just published 4.4.0 but I realise now that perhaps whats needed here is an actual major version bump. Please let me know if this is still an issue for anyone.