goatslacker / alt

Isomorphic flux implementation
http://alt.js.org/
3.45k stars 322 forks source link

Replace browserify with webpack #345

Open hipertracker opened 9 years ago

hipertracker commented 9 years ago

I am using babel-requirejs for having ES6 syntax and modules for my AMD modules. I've loaded Alt from the Bower but something was wrong. I mean, the file dist/alt-with-addons.js does not seem to be built correctly. It assumes React is global rather than the UMD module. It is not clear how to use mentioned addons. E.g. how to import connectToStores from alt-with-addons.js? That file seems to be built in the wrong way. Maybe it would be better to use Webpack to build the correct UMD module?

goatslacker commented 9 years ago

Leaving this here in case other people stumble upon this:

Alt.addons.withAltContext

hipertracker commented 9 years ago

The problem is alt-with-addons.js needs React defined globally. It does not work with React loaded by AMD. That's known bug in Browserify. It has a problem with loading packages with format "foo/bar" (e.g. "react/addons").

There are few solutions for that:

  1. (ugly and current) - adding React as global before requirejs is load and do not import React inside modules
  2. (ugly and needs addidtional change to minified version) - patching by adding two lines to generated alt-with-addons.js file

    if (typeof React === 'undefined') React = require('react'); // PATCH @ line 112

    if (typeof React === 'undefined') _react = require('react'); // PATCH @ line 2982

  3. (the best) adding --ignore-missing to browserify and rebuilding dist/*. package.json#48 should be:

    "build-alt-browser-with-addons": "browserify src/alt/addons.js -t [envify --NODE_ENV production ] -t babelify -t browserify-shim --outfile dist/alt-with-addons.js --ignore-missing --standalone Alt",
  4. replacing Browserify with Webpack which should not have such problems
hipertracker commented 9 years ago

Unfortunately, point 3 (using --ignore-missing) does not solve the problem (there is one line still trying to find React in the global namespace) I think, the best option is to rebuild that file with Webpack. It can cover all options, UMD, AMD and global.

goatslacker commented 9 years ago

I'm not opposed.

goatslacker commented 9 years ago

Here's a starting point: #222 I'm no webpack expert so someone is free to tackle this.

hipertracker commented 9 years ago

I found the line guilty for last error. Alt is loading React one place as import React from 'react/addons and in another place as import React from 'react'. This is confusing for browserify. After change to 'read/addons' and adding --ignore-missing I have no error for using var connectToStores = Alt.addons.connectToStores. But who knows if the same problem will not arise in another case... So switching to Webpack seems to be the good choice.

goatslacker commented 9 years ago

makes sense. Although I'm supposed to ignore react and react/addons

https://github.com/goatslacker/alt/blob/master/package.json#L61-L63

hipertracker commented 9 years ago

This webpack.config.js works for me:

module.exports = {
  context: __dirname + '/src',
  entry: {
    'alt': ['./alt/index.js'],
    'alt-with-addons': ['./alt/addons.js']
  },
  output: {
    path: __dirname + '/dist',
    filename: '[name].js',
    library: 'Alt',
    libraryTarget: 'umd'
  },
  module: {
    loaders: [{
      test: /\.js$/,
      loader: 'babel',
      exclude: /node_modules/
    }]
  }
};
goatslacker commented 9 years ago

Want to create a PR based off that branch?

hipertracker commented 9 years ago

Done. PR sent. But to master, not to webpack (too many commits behind).