developit / preact-redux

:loop: Preact integration for Redux (no shim needed!)
https://npm.im/preact-redux
MIT License
288 stars 26 forks source link

Fix rollup bundling #1

Closed marvinhagemeister closed 7 years ago

marvinhagemeister commented 8 years ago

I'm really intrigued by the preact community but got a bit of a trouble making preact-redux work in combination with rollup.

Bundling with rollup always leads to the following error:

Module /Users/marvinhagemeister/dev/myproject/node_modules/preact-redux/dist/preact-redux.js does not export connect (imported by /Users/marvinhagemeister/dev/myproject/src/popup.js)
Error: Module /Users/marvinhagemeister/dev/myproject/node_modules/preact-redux/dist/preact-redux.js does not export connect (imported by /Users/marvinhagemeister/dev/myproject/src/popup.js)
    at Module.trace (/Users/marvinhagemeister/dev/myproject/node_modules/rollup/src/Module.js:666:30)
    at /Users/marvinhagemeister/dev/myproject/node_modules/rollup/src/Module.js:218:35
    at Array.forEach (native)
    at /Users/marvinhagemeister/dev/myproject/node_modules/rollup/src/Module.js:215:25
    at Array.forEach (native)
    at Module.bindAliases (/Users/marvinhagemeister/dev/myproject/node_modules/rollup/src/Module.js:204:29)
    at /Users/marvinhagemeister/dev/myproject/node_modules/rollup/src/Bundle.js:95:44
    at Array.forEach (native)
    at /Users/marvinhagemeister/dev/myproject/node_modules/rollup/src/Bundle.js:95:18

I tried importing src/index.js directly and installed react-redux but that in turn wants to use react and so on...

The best solution seems to be to provide an esnext bundle of preact-redux.

developit commented 8 years ago

Hi - sorry for the slow reply! You need to add rollup-plugin-commonjs. The conversion of those react imports is part of the build process, which outputs CommonJS. I'm not aware of a way to output an esnext bundle with the aliases pre-resolved.

developit commented 8 years ago

@marvinhagemeister let me know if this fixed the issue for you 🐤

developit commented 8 years ago

Closing since no response but please re-open if it's still a problem!

yfr commented 8 years ago

Hey @developit. I still have this error. And i already included the commonjs Plugin.

Here is my rollup config:

import fs from 'fs';
import babel from 'rollup-plugin-babel';
import commonjs from 'rollup-plugin-commonjs';
import resolve from 'rollup-plugin-node-resolve';
import replace from 'rollup-plugin-replace';
import filesize from 'rollup-plugin-filesize';
import uglify from 'rollup-plugin-uglify';
import stylus from 'rollup-plugin-stylus';
import json from 'rollup-plugin-json';
import postcss from 'postcss';
import autoprefixer from 'autoprefixer';

export default {
  entry: 'src/scripts/main.jsx',
  format: 'iife',
  moduleName: 'name',
  sourceMap: (process.env.NODE_ENV === 'production' ? false : 'inline'),
  useStrict: false,
  plugins: [
    json(),
    stylus({
      output: css => postcss([
        autoprefixer
      ]).process(css, {
        map: process.env.NODE_ENV !== 'production'
      }).then(result => {
        fs.writeFileSync('build/styles.css', result.css);
      })
    }),
    babel({
      exclude: 'node_modules/**'
    }),
    resolve({
      jsnext: true,
      main: true,
      browser: true
    }),
    commonjs({include: ['node_modules/**/*js']}),
    replace({
      'process.env.NODE_ENV': JSON.stringify('production')
    }),
    (process.env.NODE_ENV === 'production' && uglify()),
    filesize()
  ],
  exports: 'none',
  dest: 'build/bundle.js'
};
developit commented 8 years ago

It seems like your commonjs config is incorrect:

// this
commonjs({include: ['node_modules/**/*js']}),
// should be
commonjs({include: ['node_modules/**/*']}),
yfr commented 7 years ago

I still get the error. This setting changes should not make any difference as there are only *js files in node_modules. Or am i missing something?

yfr commented 7 years ago

Found this issue here https://github.com/rollup/rollup/issues/517 which is dealing with a similar issue. Now i import Preact as default and assign Provider by hand.

import Redux from 'preact-redux';
const Provider = Redux.Provider;

This seems to work but is counter-intuitive. Can you explain a little what's happening?

developit commented 7 years ago

Ah, that makes sense. It's because the commonjs bundle exported here is actually just module.exports = { Provider, connect };. That's done here: https://github.com/developit/preact-redux/blob/master/rollup.config.js#L24

Your current approach actually works fine, I'm just still curious why Rollup wouldn't automatically handle this. It seems like the commonjs interop function that's supposed to get used to fall back to module.exports when exports.default doesn't exist is not being invoked.

developit commented 7 years ago

It's also possible Rollup is tripping over the UMD bundle this library produces. Maybe we should have it build an ES Modules bundle first and then export that as jsnext:main?

yfr commented 7 years ago

For the jsnext:main export can we just point to the source src/index.js? Dunno if i understood that all correctly.

developit commented 7 years ago

@yfr Sadly we cannot. The real source of this lib is the official react-redux - all this repo really does is inline it and swap the react imports out for a tiny shim. I think bundling to a single esmodules file (or Commonjs) would work though.

yfr commented 7 years ago

But would the targets option in the rollup config do the trick? Taken from the rollup-starter-project

    targets: [
    {
      dest: pkg['main'],
      format: 'umd',
      moduleName: 'rollupStarterProject',
      sourceMap: true
    },
    {
      dest: pkg['jsnext:main'],
      format: 'es',
      sourceMap: true
    }
  ]

I've just included this and it seems to be fine and to be an jsnext module.

developit commented 7 years ago

Yup, that'd work nicely. Want to PR it, or should I put it in?

yfr commented 7 years ago

No i can do this. Do we need tests for that?

developit commented 7 years ago

Shouldn't be overly necessary, but even just a test of the imports might be nice:

// tests/jsnext.js
import { Provider, connect } from '../dist/preact-redux.esm.js';
describe('jsnext:main', () => {
  it('should export the correct interface', () => {
    expect(Provider).to.be.a('function');
    expect(connect).to.be.a('function');
  });
});
ezekielchentnik commented 7 years ago

fyi, I've gotten this to work with

commonjs({ include: ['node_modules/**'], namedExports: { 'preact-redux': ['connect', 'Provider'] } }),
yfr commented 7 years ago

@ezekielchentnik Thanks, I'll try that!

ezekielchentnik commented 7 years ago

if anyone is interested here is my complete rollup config & build script: build.js gist

It uses preact, redux, preact-redux, rollup, buble, tape, sass, optimizeJS, purify-css, nodemon. Super optimized, minimal brain overload. I can post up my entire boilerplate too.

marvinhagemeister commented 7 years ago

Awesome! Thanks for sharing. 👍

ezekielchentnik commented 7 years ago

here is a demo app using: preact, redux, preact-redux, rollup, universal rendering

repo: https://github.com/ezekielchentnik/preact-pwa live demo: http://sfpwa-154005.appspot-preview.com/

ezekielchentnik commented 7 years ago

^^^ this solution is no longer working with release of 2.0.0, I'm investigating.

developit commented 7 years ago

@ezekielchentnik interesting, not sure why that would be.. there is a new exported method from the upstream react-redux@5 update called connectAdvanced - maybe related?

developit commented 7 years ago

Fixed in 2.0.2.