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

fix: make share, publish, et al. operator calls referentially transparent #6410

Merged
merged 12 commits into from
May 21, 2021

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented May 19, 2021

Description:

This PR incorporates the fixes and tests in #5585 and also fixes and tests the share, shareReplay, publishBehavior and publishLast operators.

The problem that is fixed can be described as follows:

  • Calls to operators - like share - must be referentially transparent. Otherwise, those operators cannot be used in calls to the static pipe function (to create partial pipelines).
  • It's the calls to the operator functions - the functions that are returned from operator calls - that do not have to be referentially transparent (and can therefore implement the publishing and sharing). Those calls are made when the complete pipeline is composed via a call to a source observable's pipe method.

IMO, the changes are a bug fix, rather than a breaking change. The behaviour is changed. However, the behaviour prior to this change was not sensible and I cannot see how anyone could possibly be relying upon it. Any callers using a partial pipeline composed using the static pipe function and one of the affected operators would see wildly different behaviour - subscriptions to completely unexpected sources - depending upon the order in which subscriptions were made.

Related issue (if exists): #5411 #5585

@cartant cartant added the AGENDA ITEM Flagged for discussion at core team meetings label May 19, 2021
@cartant cartant marked this pull request as draft May 19, 2021 09:22
@cartant cartant changed the title fix: make share, publish, et al. operator calls referentially transparent WIP: fix: make share, publish, et al. operator calls referentially transparent May 19, 2021
@benlesh
Copy link
Member

benlesh commented May 19, 2021

I'm generally in favor of this change. It seems like a bug that may have been introduced at some point. Can we prove that out? Is there a version of RxJS 5 or 6 where these new tests would pass?

@cartant cartant removed the AGENDA ITEM Flagged for discussion at core team meetings label May 19, 2021
@cartant
Copy link
Collaborator Author

cartant commented May 19, 2021

It seems like a bug that may have been introduced at some point

Looking at the operators in HEAD of the 5.x branch:

And it's the same with HEAD of the 6.x branch.

Going back further and looking at the first 'lettable' versions of the operators:

@cartant cartant force-pushed the cartant/referential-transparency branch from b5aa06a to 56919fa Compare May 20, 2021 22:30
@cartant cartant changed the title WIP: fix: make share, publish, et al. operator calls referentially transparent fix: make share, publish, et al. operator calls referentially transparent May 20, 2021
@cartant cartant marked this pull request as ready for review May 20, 2021 22:31
@cartant cartant requested a review from benlesh May 20, 2021 22:31
@cartant cartant added the 7.x Issues and PRs for version 7.x label May 20, 2021
@benlesh
Copy link
Member

benlesh commented May 21, 2021

So this is a regression in publishReplay introduced somewhere, and arguably a bug that has existed in publish since 5.x.

@benlesh
Copy link
Member

benlesh commented May 21, 2021

I guess the workaround for this if you really WANTED this behavior in, say, publish would be:

const olderPublish = () => {
  const subject = new Subject();
  return connect({
    connector: () => subject
  })
}

Perhaps we should document that? Or maybe the comment here will be enough. I doubt it will be an issue, TBH.

@benlesh benlesh merged commit e2f2e51 into ReactiveX:master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x Issues and PRs for version 7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants