goatslacker / alt

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

Instance `addStore` / `addActions` breaks with real classes #604

Open leebenson opened 8 years ago

leebenson commented 8 years ago

This works fine when babels transpiles with a es2015 preset.

import Alt from 'alt';

class SessionStore {
  constructor () {;
    this.bindActions(this.alt.getActions('Session'));
  }
  onUpdateId (id) {
    this.setState({
      id
    });
  }
}

class SessionActions {
  updateId (id) {
    return dispatch => {
      setTimeout(function () {
        dispatch(id);
      }, 1500);
    };
  }
}

class MyAlt extends Alt {
  constructor (config = {}) {
    super(config);
    this.addActions('Session', SessionActions);
    this.addStore('Session', SessionStore);
  }
}

module.exports = MyAlt;

However, if the Node version it's running on already has native classes, it generates the following error:

TypeError: Class constructors cannot be invoked without 'new'
   at ActionsGenerator.SessionActions (alt.js:15:1)
   at new ActionsGenerator (/Users/leebenson/dev/project/web/node_modules/alt/lib/index.js:182:92)
   at /Users/leebenson/dev/project/web/node_modules/alt/lib/index.js:205:30
   at MyAlt.createActions (/Users/leebenson/dev/project/web/node_modules/alt/lib/index.js:206:11)
   at MyAlt.addActions (/Users/leebenson/dev/project/web/node_modules/alt/lib/index.js:297:126)
   at new MyAlt (alt.js:28:10)
leebenson commented 8 years ago

Also worth mentioning: If invoking with a new prefix, the object passed through to addActions isn't bound correctly:

class MyAlt extends Alt {
  constructor (config = {}) {
    super(config);
    this.addActions('Session', new SessionActions);
    this.addStore('Session', new SessionStore);
  }
}

Error:

TypeError: Cannot read property 'getActions' of undefined
   at new SessionStore (alt.js:6:30)
   at new MyAlt (alt.js:29:30)
cannap commented 8 years ago

hmm have you created alt alt.js file with

let Alt = require('alt');
let alt = new Alt();
//Alt.debug('alt', alt);
module.exports = alt;

and import this file?

leebenson commented 8 years ago

No. I'm trying to create a new instance that extends from Alt in order to inject per-request context. It works with babel, but not bare Node 5

goatslacker commented 8 years ago

is it addStore/addActions or is it everything in this?

leebenson commented 8 years ago

I haven't tested it with other functions, but I assume anything that is handling external classes would be affected by this.

I don't think the issue is the source code. It's babel bundling.

When installing via NPM, your build step generates an alt.js and alt.min.js file, using the stage-0 plug-in. This means all classes are already treated as functions with a prototype chain. In the dist code, you then have lines like this:

Function.prototype.bind.apply(ActionsGenerator)

.. which are attempting invocation directly on a class. By this time, it's already too late - alt expects to be running in an ES5 context, whereas Node is expecting native classes. With this approach, it's all-or-nothing.

So, I thought I'd try importing the src folder directly, and forego the build step:

import Alt from 'alt/src'

It threw up a bunch of errors with the difference between Node 5's interpretation of valid ES2015 syntax, and your es-lint config (like trailing commas):

SyntaxError: /Users/leebenson/dev/sandbox/alt-test/node_modules/alt/src/store/index.js: Unexpected token (162:2)
  160 |     config.publicMethods,
  161 |     { displayName: key },
> 162 |   )

I fixed all of that stuff, and then came across missing packages from importing the NPM package locally:

Error: Cannot find module 'eslint-config-airbnb'

Fixed that by adding missing stuff to package.json, and adding the airbnb preset to my local .babelrc.

Now I'm getting a few random things that need fixing related to babel bootstrapping:

/Users/leebenson/dev/sandbox/alt-test/node_modules/babel-plugin-transform-react-jsx/lib/index.js:12
  var visitor = require("babel-helper-builder-react-jsx")({
                                                         ^

TypeError: require(...) is not a function

The source of all of this is that the lib is designed to be compiled down to ES5, and has opinionated style guides and babel bootstrapping in the source.

It's not an issue if es2015 or stage-0 is used in the parent app, since the whole thing would get compiled down to the same compatible treatment of "classes" (really just plain functions).

But then you're buying into the paradigm of compiling EVERYTHING - even stuff that Node can already do well natively, which I'd like to avoid on the server where the environment is controlled.