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(concatStatic): missing export creating breaking change #2999

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

ghetolay
Copy link
Contributor

@ghetolay ghetolay commented Oct 25, 2017

Description:

Export of concatStatic from operator/concat was removed on 5.5.
While I could easily replace :

import { concatStatic } from 'rxjs/operators/concat';

with

import { concat } from 'rxjs/observable/concat';

this is still a breaking change that could affect other projects.

@kwonoj
Copy link
Member

kwonoj commented Oct 25, 2017

This seems makes sense and we defer this change for v6 as breaking?

@rxjs-bot
Copy link

rxjs-bot commented Oct 25, 2017

Messages
📖

CJS: 2281.1KB, global: 748.1KB (gzipped: 120.4KB), min: 145.7KB (gzipped: 31.2KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0004%) to 97.399% when pulling 958ed80 on ghetolay:concatstatic_breakingchange into 24854cf on ReactiveX:master.

@ghetolay
Copy link
Contributor Author

ghetolay commented Oct 26, 2017

I looked a bit more on the code and this add inconsistency between how mergeStatic and concatStatic are handled.
mergeStatic is defined on operators/merge and reexported on both operator/merge and observable/merge while here concatStatic is defined on observable/concat, reexported on operator/concat and nothing inside operators/concat.

I was about to defined both inside observable/* and only reexport on operator/* for breaking change. But now that merge was also exported on operators/* in 5.5, removing it would create a new breaking change :)
So I'll just define them both inside observable/* and reexport on both operator/* and operators/*.
Will do that tomorrow cause I'm in need of some sleep right now 😴 .

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0007%) to 97.399% when pulling a06422e on ghetolay:concatstatic_breakingchange into 24854cf on ReactiveX:master.

@ghetolay
Copy link
Contributor Author

I think this is good to go.
We'll just have to remove the exports on operator/* and operators/* on v6.

@benlesh benlesh changed the base branch from master to stable November 3, 2017 02:34
@benlesh benlesh merged commit cae5f9b into ReactiveX:stable Dec 1, 2017
@ghetolay ghetolay deleted the concatstatic_breakingchange branch December 1, 2017 23:27
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants