Netflix / falcor

A JavaScript library for efficient data fetching
http://netflix.github.io/falcor
Apache License 2.0
10.48k stars 446 forks source link

Webpack #586

Closed IwanKaramazow closed 9 years ago

IwanKaramazow commented 9 years ago

What's the current status of the webpack issue?

I installed rx trough npm and added the following code to my webpack config

alias: {
      // Workaround https://github.com/Reactive-Extensions/RxJS/issues/832, until it's fixed
      'rx$': require.resolve('rx/dist/rx') 
    }

I get a:

ERROR in ./~/falcor/~/rx/dist/rx.aggregates.js
Module not found: Error: Cannot resolve module '12' in /pathtoproject/node_modules/falcor/node_modules/rx/dist
 @ ./~/falcor/~/rx/dist/rx.aggregates.js

Pointing the rx$ alias to 'falcor/node_modules/rx/dist/rx' doesn't solve the problem either: => webpack builds, but I get:

TypeError: this is undefined //firefox-console.

Is this an issue with Falcor, or did I mess something up?

IwanKaramazow commented 9 years ago

Update:

Tried the https://github.com/Netflix/falcor/issues/428 approach, didn't work with latest version of Rx

=> Clone Falcor
webpack --output-library falcor browser.js falcor.webpack.js

//use falcor.webpack.js in my app gives me
TypeError: "name" is read-only //firefox-console

In source this refers to:

    InvalidSourceError.prototype = new Error();InvalidSourceError.prototype.name = NAME;InvalidSourceError.name = NAME;InvalidSourceError.is = function(e){return e && e.name === NAME;};module.exports = InvalidSourceError; /***/}, /* 26 */function(module,exports,__webpack_require__){module.exports = {iterateKeySet:__webpack_require__(27),toTree:__webpack_require__(28),toTreeWithUnion:__webpack_require__(29),pathsComplementFromTree:__webpack_require__(30),pathsComplementFromLengthTree:__webpack_require__(32),hasIntersection:__webpack_require__(31),toPaths:__webpack_require__(33),collapse:__webpack_require__(34)}; /***/}, /* 27 */function(module,exports){var isArray=Array.isArray; /**
sdesai commented 9 years ago

Hey @IwanKaramazow

We're in the process of removing the internal falcor dependency on Rx so there isn't really any value in upgrading to the latest version of Rx, which has the fix for this webpack issue (there's a migration cost, since it's a major bump in the rx version and signature changes), so you'd need to stick with the workaround for Rx.

If it's the later, you'll need to point to the explicit path to rx.js from your webpack config, since falcor will be in your ./node_modules. See the 'Workaround Notes' here. You can try literally hardcoding the path to rx.js (instead of using require.resolve) to see if that's actually the issue.

Alternatively if you don't use AMD for your modules, you could try disabling webpack's AMD parsing: See: https://github.com/webpack/imports-loader

a. npm i imports-loader b. Add the following to your webpack.config in the module.loaders section:

 {
   // configure the loader for all js files (you can configure this differently if required
   test: /\.js$/, 
   // disable webpack's AMD parsing.
   loader: "imports?define=>false"
}
IwanKaramazow commented 9 years ago

Thanks for the response!

I'm trying to build with Falcor as a dependency.

Seems this fixes the original issue:

//Changing the AMD line in rx.aggregate's header to:
define(['./rx'], function (Rx, exports) {

but...

the following code breaks in rx.js:

var inherits      //= this.inherits   (I have to comment the this.inherits out to make it work, this is undefined)
  = Rx.internals.inherits = function (child, parent) {
    function __() { this.constructor = child; }
    __.prototype = parent.prototype;
    child.prototype = new __();
  };

Next file that breaks: falcor/lib/errors/InvalidSourceError.js

InvalidSourceError.name = NAME; //TypeError: "name" is read-only

Same thing happens with InvalidModelError.js, MaxRetryExceededError.js

Known issue?

I think for now I'll insert a script tag w/"https://netflix.github.io/falcor/build/falcor.browser.js" Seems this does the job just fine, only problem is my app has to be production ready in eight weeks. ;-)

sdesai commented 9 years ago

I'd be worried about commenting out the this.inherits line.

If your project is available on github, I can try and help you configure it using one of the methods I mention above. It should be pretty straight forward.

The InvalidSourceError is not a known issue, but if you can file it, we can take a look at it.

Which version of Firefox is this, and is it the only browser you're seeing it on?

In theory (as per ES6 spec) I believe function.name is read-only, but I don't see that being enforced in Chrome (46.x) or FF (40.0.2)

IwanKaramazow commented 9 years ago

I'm using firefox 41.0.2, I suspect it has something to do with commenting out too much code. Probably isn't worth filing an issue.

If you could spare some time, it would be really nice if you could take a look at my project. It's a private one: I added your username. https://github.com/IwanKaramazow/VeggieApp/tree/webapp/feature/refactor-quiz/Webapp (currently all code lives in webapp/feature/refactor-quiz branch) The webpack config (frontend) is located in the /config folder

sdesai commented 9 years ago

Looked at your repo. Unfortunately all of these hacks are required.

1) The function.name issue is real.

If you file it we can take care of it. If you need to work around it locally for now, you can uncomment the lines which set .name on the constructor function for everything in falcor/lib/errors. The browsers enforce .name being read-only when using "use strict".

We should be able to fix this. I'm not sure the .name is actually required (depending on our browser support matrix).

2) The this.inherits fix is also required unfortunately. It is safe. It's actually the fix which went into Rx 3.x.

The root issue here is really a Babel + RxJS issue.

It looks like Babel runs with an undefined "this" as the top level context.

The only way we'd be able to address this in Falcor is once Rx 2 is removed (or upgraded to an another version of Rx).

I'm not sure if there's a Babel configuration you could use to control this (to have Babel allow you to configure the top level context, but that may let you work around it).

See: https://github.com/Reactive-Extensions/RxJS/issues/850 See: https://github.com/Reactive-Extensions/RxJS/commit/7f118a46d73ee122bbb3335dfccf746af9fbd75b#diff-b9a1e3a1f7a1cabdc218ed6d13633a67L539

3) For the "webpack" issue where you started, the imports-loader would be the best way to handle this. I tried this out, and it does run fine, but I'd need to spend some time to figure out how to get it to work in WebPack with your other loaders (Babel etc).

If I take care of these issue your app builds/works fine with the local falcor. I'll let you know if I can figure out how to get c working.

sdesai commented 9 years ago

For c) above, this fixes it also, in your build env...

[ in webpack.config.frontend.js ]

resolve: {
    // you can now require('file') instead of require('file.js')
    extensions: ['', '.js','.json'],
    alias: {
      // Workaround https://github.com/Reactive-Extensions/RxJS/issues/832, until it's fixed
      'rx$': path.join(__dirname, '../node_modules/falcor/node_modules/rx/dist/rx.js')
    }
  },

For whatever webpack/build pipeline reason (and you have a pretty complex build pipeline), the require.resolve() at the point in time at which your webpack.config is processed just gives you back the webpack id of the module (e.g. '12' for rx/dist/rx) instead of the pull path to the file.

Alternatively, you can do this, to disable AMD parsing for rx.aggregates.js (or other files if you want). This also fixes the issue with rx.aggregates.js ...

    loaders: [
      { test: /rx\.aggregates\.js$/, loader: "imports?define=>false"},
      { test: /\.js$/, loaders: ['babel'], exclude: 'node_modules'},

For a) and b) there may be something in Babel's config you can do to stop it.

  1. Inserting the "use strict"
  2. Setup a non-undefined global 'this' reference.

To avoid all the workarounds, at least until we can remove Rx and fix the .name issue.

IwanKaramazow commented 9 years ago

Thanks for the input, really appreciate it!

I somehow got it the RX dependency working, but... 😁

/falcor/node_modules/falcor-path-syntax/src/tokenizer/index.js line 73.
        done = idx + 1 >= string.length; //ReferenceError: assignment to undeclared variable done

Should probably be var done = idx+1 >=string.length;, can't find the declaration of done...

I'll go with a <script src="https://netflix.github.io/falcor/build/falcor.browser.js"></script> in my index.html for now. After fixing the undeclared variable, another error rises.. I don't want to go too far down this rabbit hole. Gotta love the fragile javascript/webpack/babel ecosystem 😅

Thanks anyway for the help! 👌

sdesai commented 9 years ago

The latest falcor (0.1.14) and falcor-router should have all these fixes [ function.name, global leaked done and i ]. I tried it out locally, with a 'use strict' and the falcor-express-demo seems fine.