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

7.0.0-beta.9 conflicts symbol.observable to redux (and other have old symbol-observable version) #5919

Closed
kwonoj opened this issue Dec 7, 2020 · 12 comments · Fixed by #6224
Assignees
Labels
AGENDA ITEM Flagged for discussion at core team meetings bug Confirmed bug TS Issues and PRs related purely to TypeScript issues

Comments

@kwonoj
Copy link
Member

kwonoj commented Dec 7, 2020

Bug Report

Current Behavior
A clear and concise description of the behavior.

repro at https://github.com/kwonoj/rxjs-symbol-test, npm i && npm run build will raise compilation error.

image

#5874 introduces changes to symbol, which also updated in symbol-observable to support this (https://github.com/benlesh/symbol-observable/blob/master/CHANGELOG.md#300-2020-11-02). But some of packages, for examble redux (https://github.com/reduxjs/redux/blob/v4.0.5/package.json#L53) relies on older version of symbol-observable conflicts with latest rxjs changes.

This is particularly tricky as there's no easy workaround instead of trying to mutate module resolution somehow.

Expected behavior
A clear and concise description of what you expected to happen (or code).

Environment

  • Runtime: [e.g. Node v${x}, Chrome v${x}]
  • RxJS version:
  • (If bug is related) Loader, build configuration: [e.g webpack, angular-cli version, config]

Possible Solution

Additional context/Screenshots
Add any other context about the problem here. If applicable, add screenshots to help explain.

@kwonoj kwonoj added bug Confirmed bug TS Issues and PRs related purely to TypeScript issues labels Dec 7, 2020
@benlesh
Copy link
Member

benlesh commented Dec 7, 2020

Ugh. I guess we can change it back to symbol. I'm just not sure how we can ever get that type right. I guess we can't. :\

@benlesh
Copy link
Member

benlesh commented Dec 7, 2020

Or we could upstream other changes... maybe deprecate symbol-observable and try to get people to move to 3.x of that library?

@kwonoj
Copy link
Member Author

kwonoj commented Dec 7, 2020

get people to move to 3.x of that library

That's only pass I can see as well, since I don't see easy workaround. But problem is it's really long tail, i.e we can't say for sure how long will it take the majority of redux user upgrade to latest to get away with old symbol-observable.

Maybe it's worth to ask redux upstream at least to publish latest version to remove / or update symbol-observable? Not immediate solution but will help.

@kwonoj
Copy link
Member Author

kwonoj commented Dec 7, 2020

redux upstream at least to publish latest version to remove / or update symbol-observable? Not immediate solution but will help.

at least if we have published version we can recommend to upgrade redux as well.

@benlesh
Copy link
Member

benlesh commented Dec 7, 2020

Well, really the issue is if they're using redux AND rxjs, (so perhaps worth pulling in @jayphelps because redux-observable).

@kwonoj
Copy link
Member Author

kwonoj commented Dec 7, 2020

redux-observable is safe. I specifically meant redux cause latest release have symbol-observable (https://github.com/reduxjs/redux/blob/v4.0.5/package.json#L53) and looks like master seems safe from it (https://github.com/reduxjs/redux/blob/master/package.json#L53), but there isn't version published with these changes.

@benlesh
Copy link
Member

benlesh commented Dec 7, 2020

At this point I think maybe the best answer is that RxJS just sort of "shadow supports" Symbol.observable. In other words, since it's a crap shoot that TypeScript will even figure out the right thing to do, perhaps we don't add the type or type handling, and instead we only handle the proper runtime behavior. I just don't know.

I seriously doubt anyone has ever gotten this working properly in TypeScript haha.

Just look at how confusing this is:

https://www.typescriptlang.org/play?target=99#code/JYOwLgpgTgZghgYwgAgMoE8C2AjA9gGwGFcQBnMKAVwTFymQG8BYAKGXeSgjgBMT90yXNlLQAbnGz4IALmSUQwAI6UUpLHnwBuVgF9WrUJFiIUAeRHjoAHgAqAPkasOyEBAAeYABQT8qubYAlHJiuMA8Omwc0FB0XjFycCDowcih4ZEuCLiYAA7SkF6p6RF6BixG0PBIyACqZJQiCFDA2JLSTlHsCqSNpM2tEEUhYaUs+iyG4FWmaH0DbVIQdo7MXci9TS3YQ8KiUGLQchb7h1ArqfWb-dvtEJET5ZUmNSfid6Qqyw6dLgDaGBwBAAdHt3ksALrDOZbVp3FYPIA

@benlesh
Copy link
Member

benlesh commented Dec 7, 2020

redux-observable is safe. I specifically meant redux cause latest release have symbol-observable (https://github.com/reduxjs/redux/blob/v4.0.5/package.json#L53) and looks like master seems safe from it (https://github.com/reduxjs/redux/blob/master/package.json#L53), but there isn't version published with these changes.

Specifically, I mean people using redux-observable will have this issue because they're definitely using both redux and rxjs.

@jayphelps
Copy link
Member

If redux is willing to patch, I don't personally see it as a particularly egregious issue that people would have to update their redux to be able to use the latest rxjs. Things like that happen.

@kwonoj
Copy link
Member Author

kwonoj commented Dec 7, 2020

I mean people using redux-observable will have this issue because they're definitely using both redux and rxjs

that is true. it was my case 😭

@jayphelps
Copy link
Member

Yeah, certainly not ideal if it can be avoided. I don't have full background to comment on that, though.

@kwonoj kwonoj added the AGENDA ITEM Flagged for discussion at core team meetings label Dec 16, 2020
@benlesh
Copy link
Member

benlesh commented Feb 10, 2021

We should revert to whatever we were doing before. Which is just having symbol and not unique symbol. ... symbol-observable will also need to be rolled back.

@benlesh benlesh self-assigned this Mar 24, 2021
kwonoj added a commit that referenced this issue Apr 12, 2021
kwonoj added a commit that referenced this issue Apr 15, 2021
benlesh pushed a commit that referenced this issue Apr 15, 2021
* fix(symbol): revert unique symbol in #5874

- closes #5919

* fix(types): move Symbol.observable into types.ts (#6178)

Closes #6175

Co-authored-by: Nicholas Jamieson <nicholas@cartant.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AGENDA ITEM Flagged for discussion at core team meetings bug Confirmed bug TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants