Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

RxJS throws errors when building with WebPack - UMD Header Bug? #832

Closed
sdesai opened this issue Jul 24, 2015 · 8 comments
Closed

RxJS throws errors when building with WebPack - UMD Header Bug? #832

sdesai opened this issue Jul 24, 2015 · 8 comments

Comments

@sdesai
Copy link

sdesai commented Jul 24, 2015

When using RxJS with WebPack, apps cannot require('rx/dist/rx') along with incremental bundles, such as rx/dist/rx.aggregates, due to the UMD header added to rx/dist/rx.aggregates et al pulling in and modifying rx/dist/rx.all as a dependency instead of rx/dist/rx.

Full details, and repo case here: https://github.com/sdesai/rxjs-webpack-test

NOTE: I realize there have been other issues and threads around this issue, but I don't believe the root cause has been fixed (outlined in the link above).

@trxcllnt
Copy link
Contributor

@sdesai could you submit a PR with your proposed change?

@sdesai
Copy link
Author

sdesai commented Jul 31, 2015

I can, although my proposed change may break something else (The define(['./rx'] change), since looking at some of the repo history, it seems like it used to be ./rx at some point, but was changed to rx to fix another issue: Reactive-Extensions/RxJS-DOM@a74e523#diff-f1ebe0c2ce0a72c9131e854556761642L23

Will submit it, but again not sure what impact is has on all potential use cases/builds.

@s-petersson
Copy link

The following error was also received when compiling with webpack.

ERROR in ./~/rx/dist/rx.async.js
Module not found: Error: Cannot resolve module 'rx.binding' in
    /path/to/project/node_modules/rx/dist @ ./~/rx/dist/rx.async.js 25:8-28:10

ERROR in ./~/rx/dist/rx.testing.js
Module not found: Error: Cannot resolve module 'rx.virtualtime' in
    /path/to/project/node_modules/rx/dist @ ./~/rx/dist/rx.testing.js 25:4-28:6

I fixed it with the following modifications:
dist/rx.async.js

define(['rx.binding', 'exports'] -> define(['./rx.binding', 'exports']

dist/rx.testing.js

define(['rx.virtualtime', 'exports'] -> define(['./rx.virtualtime', 'exports']

Im not confident enough with RxJS just yet to create a PR with this. Could there be any side-effects to this?

mattpodwysocki added a commit that referenced this issue Aug 17, 2015
@mattpodwysocki
Copy link
Member

@sdesai @trxcllnt @Pendla can you all test what I have now to see if it works?

@sdesai
Copy link
Author

sdesai commented Aug 19, 2015

@mattpodwysocki - Has this been pushed to npm yet?

Just tried retesting my original repro case (above) and it's still failing, but it's pulling down rx@3.0.1, and the dist in that version doesn't seem to have the change (I checked node_modules/rx/dist/rx.aggregates.js and it's still define['rx']).

Were you waiting for the testing feedback before publishing? If so, I can change the repro case to pull in Rx from github (same for falcor)

@sdesai
Copy link
Author

sdesai commented Aug 19, 2015

The basic repro I had now passes, if I pull in the lastest 'master', so looks good there. I'll try it out with Falcor and update.

@sdesai
Copy link
Author

sdesai commented Aug 19, 2015

A webpack build works fine with the Falcor demo server too (https://github.com/netflix/falcor-express-demo) without the special webpack.config.js (containing the alias workaround).

@mattpodwysocki
Copy link
Member

@sdesai ok, great, then I will get it out now.

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

No branches or pull requests

4 participants