goatslacker / alt

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

StoreModel.config in ES5? #233

Closed beverlycodes closed 9 years ago

beverlycodes commented 9 years ago

I'm having a difficult time overriding getState and setState in ES5 syntax. I tried a bunch of variations of the following:

function StoreClass() { }

StoreClass.config = {
  getState: function (state) { }
  setState: function (state, nextState) { }
}

Neither function ever gets called. I've also tried setting them directly on StoreClass, at which point they get called, but don't contain a proper this reference. I'm sure this is something I'm just not understanding about ES6 static, but I'm at a loss at this point. Any help would be appreciated.

goatslacker commented 9 years ago

What version of alt are you running?

This works for me:

const Alt = require('alt')

const alt = new Alt()

function StoreClass() { }

StoreClass.config = {
  getState: function (state) { console.log('@@@@@@@@@'); return state }
}

const store = alt.createStore(StoreClass)

console.log(store.getState())
beverlycodes commented 9 years ago

I was still on an 0.15 version. I've upgraded to 0.16.2, but I still can't tell if my issue is resolved because a new issue has cropped up. In 0.15, I had access to Alt.addons.ReactStateMagicMixin if I used alt-browser-with-addons.js. The new alt-with-addons.js doesn't appear to include that mixin, and I don't have another way to include it because I'm in a build environment which isn't compatible with require statements. Was ReactStateMagicMixin removed intentionally?

UPDATE

I tried migrating over to AltContainer, but that seems to have opened up a new can of worms on the server side of things. While doing server-side rendering, an attempt to require('alt/AltContainer') fails with Error: Cannot find module 'react/addons'. Should React be more than just a dev dependency?

goatslacker commented 9 years ago

I removed them because AltContainer is the way forward and to reduce code bundle size. If you really want mixins you can browserify them yourself so I thought.

Should

goatslacker commented 9 years ago

Ugh mobile...

Should I add them back for convenience? Do you care about bundle size? Should I provide a way for people to browserify them?

In regards to AltContainer react is required however I don't want to express a peer dependency or actual dependency in alt of React because you could be running different versions of react. Also, if you were using mixins before, don't you already have React?

beverlycodes commented 9 years ago

I'm working inside Meteor, which isolates packages from each other. When I bring in Alt as a meteor package, it only sees its own NPM dependencies. I can add React as a dependency to the Meteor package, but it introduces the exact same problem you just described. My Alt package will have a specific React dependency that may not match up with the React dependency used directly by my app.

I like AltContainer, but its implicit React dependency is problematic in Meteor's build system. ReactStateMagicMixin worked because it had no direct dependency on React. Can AltContainer be written as a mixin? I had already started migrating to component containers. Being able to drop an AltContainer mixin into my containers would be cleaner than having to wrap the contents of each of my containers in a somewhat redundant AltContainer.

goatslacker commented 9 years ago

Bits of AltContainer already exist as a mixin but a lot of the power comes from it being a higher-order component and not a mixin. I haven't worked with meteor before, this sounds awful :(

Perhaps I can include a special meteor package or dependency file?

beverlycodes commented 9 years ago

The Meteor devs made some questionable decisions for their build process. If they hadn't done such a fantastic job with their socket publishing implementation, I wouldn't be using Meteor at all.

I'm not sure there's an ideal solution here. I think I may just consider this closed and switch back to Omniscient for now.

goatslacker commented 9 years ago

I'll have a look at how meteor bundles files and see if I can provide a solution.

beverlycodes commented 9 years ago

That's a deep rabbit hole. Let me know if you have questions, as I've been in the thick of it for a while now.

goatslacker commented 9 years ago

@ryanfields would something like this work: https://github.com/goatslacker/meteor-alt ??

beverlycodes commented 9 years ago

@goatslacker No, because the npm support in Meteor is not isomorphic. Npm managed packages are only available on the server, and the Npm.require function does not exist on the client side. In order to load alt client-side, you have to provide the browser-ready version of Alt to the client, and then export whichever symbol Alt would export to the window object when loaded in the browser.

I've managed to assemble a package that accomplishes that, but while Alt seems to be exported up to window, AltContainer does not.

See https://github.com/goatslacker/meteor-alt/pull/1 for a package that works except for the AltContainer export, which is undefined.

beverlycodes commented 9 years ago

Ugh. Scratch that. The package doesn't even work server-side, again because of the implicit React dependency. If you add React to the Npm.depends call, it resolves the issue server-side, but as mentioned previously, introduces an explicit React dependency that may be at odds with the version of React used by any app the package is included in.

I just don't think Alt and Meteor can play nicely together. It wouldn't be the first case of this happening, and certainly won't be the last. This is more the fault of Meteor than Alt.

goatslacker commented 9 years ago

i could provide an altcontainer that is prepackaged to use window.React or global.React.

Any suggestions on how to accomplish this isomorphic ally for meteor?

It sounds like something we can make work.

beverlycodes commented 9 years ago

To make this work client side, yes, you would have AltContainer make use of window/global React.

The bigger issue is on the server-side. There, you're just loading Alt via npm, and that's where missing React is the bigger challenge. On the server side, your AltContainer needs to get React via require('react'). In a CommonJS world, you don't have any other choice. This is where the dependency issue comes into play. In Meteor, you do not have access to npm modues that you don't explicitly declare. It's a serious failing on the part of Meteor to have set up npm support that way, but there's no indication that it's changing anytime soon. If you do not declare React as an explicit dependency, your server-side require('react') fails. If you do declare React as an explicit dependency, you'll be embedding a version of React that won't always match the parent application's version of react.

Due to Meteor's awkward choices for npm support, I don't think there's a good solution here.

goatslacker commented 9 years ago

Wow how do people even import any packages in meteor?

beverlycodes commented 9 years ago

This is something of an edge case, where there's a implicit dependency that's a "framework" dependency. React is needed at the top level and all incoming packages would like to just use that version.

There are two other dependency scenarios which are more common, but neither fits Alt's dependency on React. The first is that all dependencies are explicit. The second is that there is an implicit dependency, but it's filled by the Meteor package. We can do the following:

Npm.depends({
  alt: '0.16.2',
  react: '0.13.3'
});

This brings React into the package's node_modules folder. Your calls to require('react') will work. However, you'll be locking in React '0.13.3' while the top level app could be using any past, current, or future version of React.

I'm betting that this sort of framework dependency is a rare case for Meteor apps, and that the other two dependency scenarios mentioned above cover the majority of dependency cases.

Like I said at the start, Meteor's package system is a giant can of worms. I wouldn't be using Meteor at all except that they're the only framework right now that provides a sane and secure websocket publishing model which is critical for the realtime app I'm building.

goatslacker commented 9 years ago

That's not too bad -- locking react at 0.13.3. The problem then becomes keeping react at latest. I tried searching to see if there was a concept of peerDependencies or dependency deduplication but haven't found anything.

beverlycodes commented 9 years ago

It's functional, but awkward. Worse because I'm already using a package that brings React in (as will most people who're using React with Meteor). At some point, that package will update to React 0.13.3, but right now it's on a slightly older version. So in my project, I'd have at least two copies of React operating server-side. This remains true even if both packages use the same version, because both will bring in React independently from separate node_modules paths.

goatslacker commented 9 years ago

Closing because meteor packaging is :(