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

Remove webpack notice #458

Closed
wants to merge 2 commits into from
Closed

Remove webpack notice #458

wants to merge 2 commits into from

Conversation

adi-ads
Copy link

@adi-ads adi-ads commented Aug 19, 2015

RxJS issue has been resolved and released, tested with falcor and works well.

RxJS issue has been resolved and released, tested with falcor as well and works well.
@dashed
Copy link

dashed commented Aug 19, 2015

If it works, shouldn't this be removed then? https://github.com/Netflix/falcor/blob/master/webpack.config.js

@adi-ads
Copy link
Author

adi-ads commented Aug 19, 2015

Good point, deleted.

@sdesai
Copy link
Contributor

sdesai commented Aug 19, 2015

Did you have a chance to test this off of npm?

When I looked at it last night, it was fixed on RxJS#master, and I tested it from there (Reactive-Extensions/RxJS#832 (comment)), but wanted to make sure it's working off of npm, out of the box, before removing the notice and config.

@sdesai
Copy link
Contributor

sdesai commented Aug 19, 2015

It's also worth noting that this will bump up the RxJS dep from 2.x to 3.x, so that may need some validation, outside of the webpack issue [ unit tests, performance, api changes etc. ]

@adi-ads
Copy link
Author

adi-ads commented Aug 20, 2015

Only did it with local git clones, will try out with npm

@sdesai
Copy link
Contributor

sdesai commented Aug 20, 2015

Cool. Thx. Although as mentioned above we may need to gate removing the webpack notice/config on determining whether or not we want to upgrade to RxJS 3.x in general, which is a broader decision, outside of the WebPack issue (for example #464)

@dallonf
Copy link

dallonf commented Aug 21, 2015

This fix is still needed for the version of RxJS listed in package.json, FYI, otherwise you'll run into bizarre issues where Observables don't have reduce() functions and the like.

Don't be like me and assume you don't need the fix just because this ticket exists :)

@sdesai
Copy link
Contributor

sdesai commented Oct 28, 2015

I'm closing this out. We'll need the notice in place until we complete removal of RX as a core falcor dependency. See: #465

@sdesai sdesai closed this Oct 28, 2015
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.

4 participants