shatran / card-react

React component for card https://github.com/jessepollak/card
MIT License
133 stars 65 forks source link

Does not build under webpack #2

Closed pospi closed 8 years ago

pospi commented 9 years ago

Webpack seems to be very particular about loading dependencies with multiple module definitions in them. Though it presents as a warning, the warning does in fact break a build from completing in production mode:

WARNING in ./~/card-react/~/payment/lib/payment.js
Critical dependencies:
1:459-466 This seems to be a pre-built javascript file. Though this is possible, it's not recommended. Try to require the original source to get better results.
 @ ./~/card-react/~/payment/lib/payment.js 1:459-466

There are also problems with payment itself which need to be resolved before fully fixing this issue, see https://github.com/jessepollak/payment/pull/16

PR incoming to fix the issue for card-react.

pospi commented 9 years ago

It should also be mentioned that the above fixes won't actually resolve issues when including the module as-is, only when including the cjsx source. You'll have to configure webpack to define a browser-check environment variable and support coffeescript/react transforms; then you should be able to use something similar to the below to work around the issue:

var ReactCardFormContainer;
if (process.env.IS_BROWSER) {
  ReactCardFormContainer = require('card-react/src/components/card-react-form-container.cjsx');
} else {
  ReactCardFormContainer = require('card-react');
}
vitalybe commented 9 years ago

What is that for?

} else {
  ReactCardFormContainer = require('card-react');
}

Why not to require cjsx always directly?

pospi commented 9 years ago

It's to do with the way webpack checks files when building. My understanding is basically-

shatran commented 9 years ago

@pospi i'm kinda new to webpack, so i'll ask you this: If I'll change card-react.js file in the root folder from

module.exports = require('./lib/card-react-form-container');

to

var ReactCardFormContainer;
if (process.env.IS_BROWSER) {
  ReactCardFormContainer = require('src/components/card-react-form-container.cjsx');
} else {
  ReactCardFormContainer = require('lib/card-react-form-container');
}
module.exports = ReactCardFormContainer;

and will change the main file in package.json to card-react.js

will it allow webpack to include the module as-is? and in the same time not interfering with the non-webpack require?

pospi commented 9 years ago

Hey there, after looking into it unfortunately things are a bit more complicated than that. Definitely leave your main export as-is.

I should have mentioned in my earlier post, but IS_BROWSER is a check specific to the app I'm currently working on. Webpack provides a plugin for setting variables in your compilation process, so the way I determine whether I'm running things serverside in node vs. clientside via a compiled webpack build is to use DefinePlugin to set an environment variable I can switch on.

It's a workaround to be sure, but it's enough to get webpack running. The real issue is that @jessepollak has precompiled payment's modules (payment itself and qj) all into the same file, and I believe it's multiple instances of module.exports within one source file that webpack specifically complains about (it's not technically valid node module structure either, but it'll work).

The real fix would be for all modules to compile their files into separate source files and to reference each other separately, so the final build of payment should only include its own source in payment/payment.js and still reference QJ by requireing directly from node_modules. If that change was completed then card-react would be able to load payment by its main entrypoint under both node and webpack without issues.

I'll leave it up to you guys as to whether you think it's worth fixing, with things as they are at the moment it's possible to get both your modules working under either environment so I'm happy either way (:

shatran commented 9 years ago

@pospi I've released version 1.0.18 a few days ago with the merged fix and with the new payment version. I'll be happy to know if it worked for you. Thanks!

nicoecheza commented 8 years ago

Hi @shatran We are using this module, but we are still having the "Not found module payment/src/payment" which seems pretty reasonable since we are using v3 of npm and the "payment" dependency it's being installed independently from card-react. For now, we bypassed the error editing the "card-react-form-container.js" and "card-react-component.js" files to just require "payment" as an independent module (Payment = require('payment')). But it would be nice to have a solution for this.

Yes, we are using webpack too :dancer:

shatran commented 8 years ago

HI @nicoecheza, Thanks for your comment. are you using version 1.0.18 of card-react? i added payment there as a dependency.

nicoecheza commented 8 years ago

Yes, we are.

shatran commented 8 years ago

I've decided to restore the old require (Payment = require('payment'). I think that's the right way, and like @pospi said "The real fix would be for all modules to compile their files into separate source files and to reference each other separately". so by doing this + hoping the payment requiring qj issue will be solved, it should work fine. @nicoecheza please use version 1.0.19 and let me know if it solved your issue.

nicoecheza commented 8 years ago

@shatran nice, thanks you!