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 rxjs import #119

Closed
wants to merge 1 commit into from
Closed

Fix rxjs import #119

wants to merge 1 commit into from

Conversation

vlad-zhukov
Copy link
Contributor

What: rxjs uses named exports, but prettier-eslint-cli imports it as a default.

Why: prettier-eslint-cli doesn't work because it throws with a TypeError: Cannot read property 'Observable' of undefined.

How: By replacing a default import with a wildcard one.

Tested against rxjs@5.3.0 and rxjs@5.5+.

@codecov-io
Copy link

codecov-io commented Dec 2, 2017

Codecov Report

Merging #119 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #119   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         108    108           
  Branches       14     14           
=====================================
  Hits          108    108
Impacted Files Coverage Δ
src/format-files.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2c21fa...6d5708d. Read the comment docs.

@zimme
Copy link
Member

zimme commented Dec 2, 2017

Thanks for this. I'll have to do some testing for backwards compatibility, I'm hoping I can get it merged tomorrow.

@zimme
Copy link
Member

zimme commented Dec 2, 2017

Seems like there's a breaking change in rxjs 5.5.2 -> 5.5.3 where the export isn't available any more. ReactiveX/rxjs#3151.

Let's hold off until tomorrow to see if they can fix this.

@nataly87s
Copy link

if you change the import to
import { Observable } from 'rxjs'
and use Observable instead of Rx.Observable it should be compatible with previous versions as well

@zimme
Copy link
Member

zimme commented Dec 3, 2017

I was hoping that was the case. I'll have a few hours soon to work on these packages. So I expect to release a new version later tonight.

@zimme
Copy link
Member

zimme commented Dec 3, 2017

Merged in 239de8f.

@zimme zimme closed this Dec 3, 2017
@SivaPrasanna45
Copy link

Where i need to change @nataly87s Please let me know. in my code i dont have any import {observable} they are all coming from nodeModules itself. That's why i confused.

Thanks in advance.

@zimme
Copy link
Member

zimme commented Dec 4, 2017

@SivaPrasanna45 You should only need to update this package to get the fix.

@SivaPrasanna45
Copy link

@zimme I have already updated packages itself, in case need to update what i need to update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants